Skip to content

Conversation

@kjdelisle
Copy link
Contributor

Part of #914

Add the capability to set basePath as a part of your config.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Related API Documentation was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

@kjdelisle
Copy link
Contributor Author

kjdelisle commented Jan 26, 2018

Keep in mind that this requires more unit tests, which I'll add later.
EDIT: This also does not include the functional changes that make use of the configuration, yet.

@kjdelisle kjdelisle force-pushed the rest/allow-basePath-config branch from 718e615 to 93dd326 Compare January 26, 2018 23:49
export interface RestServerConfig {
host?: string;
port?: number;
basePath?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any code to use server-level basePath? Is it to be implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be a part of a subsequent commit in this PR, but it'll need to wait until @jannyHou lands #916

@jannyHou
Copy link
Contributor

hi @kjdelisle there is some change for the basePath in PR #916 , I believe it's highly twisted with the problem you are addressing. I just realized the controllerSpec also has a basePah, thought it is the same as openapiSpec level one but it's not.

Could you hold on adding more code and we do a pair programming next week? Thanks :)

@bajtos
Copy link
Member

bajtos commented Jan 29, 2018

IMO, this is not needed for our MVP, therefore this pull request should be closed until MVP is done. If we consider non-configurable basePath as a problem, then let's fix the docs please.

@kjdelisle
Copy link
Contributor Author

This PR can probably be closed, since the final form of what we're working with will change based on @jannyHou 's upcoming update to move us to OpenAPI v3.

@kjdelisle kjdelisle closed this Jan 29, 2018
@jannyHou
Copy link
Contributor

@kjdelisle I had a talk with @bajtos this morning, most of the basePath in rest server component refers to controller level basePath, not app level basePath, so I am not going to address this feature(configured app level basePath) in the v2-->v3 PR.

And please note in the coming openapi3 spec, there is no root level property called basePath, it is merged into servers[{url: '<a_url>'}]

@bajtos bajtos deleted the rest/allow-basePath-config branch June 5, 2018 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants