Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rest): correctly handle basePath set via basePath() API #3266

Merged
merged 1 commit into from Jul 4, 2019

Conversation

@bajtos
Copy link
Member

commented Jun 28, 2019

  • Fix server.basePath() method to update basePath in the config object too.
  • This fixes the server url in openapi.json to include basePath again.
  • Add more test to verify the changes and prevent future regressions

See #914

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

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

馃憠 Check out how to submit a PR 馃憟

@bajtos bajtos added bug REST labels Jun 28, 2019

@bajtos bajtos requested review from raymondfeng, nabdelgadir and sanadHaj Jun 28, 2019

@bajtos bajtos self-assigned this Jun 28, 2019

@bajtos bajtos referenced this pull request Jun 28, 2019
@nabdelgadir
Copy link
Contributor

left a comment

I tried it by adding this.basePath('/api') to application.ts in the todo example, but it's still not updating it in /openapi.json:

Screen Shot 2019-06-28 at 9 04 55 AM

----------

Screen Shot 2019-06-28 at 9 07 16 AM

Here's the explorer (the url is using /api):

Screen Shot 2019-06-28 at 9 08 40 AM

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

I tried it by adding this.basePath('/api') to application.ts in the todo example, but it's still not updating it in /openapi.json:

That's weird! Did you rebuild the entire monorepo?

Here is how I am editing examples/todo/src/application.ts myself:

diff --git a/examples/todo/src/application.ts b/examples/todo/src/application.ts
index 9ee0dc68..566e3137 100644
--- a/examples/todo/src/application.ts
+++ b/examples/todo/src/application.ts
@@ -18,6 +18,8 @@ export class TodoListApplication extends BootMixin(
   constructor(options: ApplicationConfig = {}) {
     super(options);

+    this.basePath('/api');
+
     // Set up the custom sequence
     this.sequence(MySequence);

And here is a screenshot from the API Explorer:

Screen Shot 2019-07-02 at 12 06 15

@nabdelgadir can you try one more time please?

@bajtos bajtos force-pushed the fix/openapi-basepath branch from 89913aa to 6ccf661 Jul 2, 2019

@nabdelgadir
Copy link
Contributor

left a comment

LGTM 馃憤

fix(rest): correctly handle basePath set via basePath() API
- Fix `server.basePath()` method to update `basePath` in the config
  object too.
- This fixes the server url in openapi.json to include basePath again.
- Add more test to verify the changes and prevent future regressions

Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>

@bajtos bajtos force-pushed the fix/openapi-basepath branch from 6ccf661 to c41a2fa Jul 4, 2019

@bajtos bajtos merged commit 2118d80 into master Jul 4, 2019

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.01%) to 91.485%
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@delete-merged-branch delete-merged-branch bot deleted the fix/openapi-basepath branch Jul 4, 2019

@nabdelgadir nabdelgadir added this to the July 2019 milestone milestone Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.