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’ll 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

@bajtos bajtos 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 Issues related to @loopback/rest package and REST transport in general labels Jun 28, 2019
@bajtos bajtos requested review from raymondfeng, nabdelgadir and a user June 28, 2019 08:47
@bajtos bajtos self-assigned this Jun 28, 2019
@bajtos bajtos mentioned this pull request Jun 28, 2019
Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

bajtos 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 Compare July 2, 2019 10:08
Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

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

LGTM 👍

- 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š <[email protected]>
@bajtos bajtos force-pushed the fix/openapi-basepath branch from 6ccf661 to c41a2fa Compare July 4, 2019 07:11
@bajtos bajtos merged commit 2118d80 into master Jul 4, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/openapi-basepath branch July 4, 2019 07:44
@nabdelgadir nabdelgadir added this to the July 2019 milestone milestone Jul 4, 2019
@AnanthGopal
Copy link

Hi @bajtos
I added the this.basePath('/api') into application.ts but it's still not updating it in /openapi.json

@AnanthGopal
Copy link

AnanthGopal commented Feb 6, 2020

hi @raymondfeng ,
When releasing this feature? As of now, i can't use swagger because i added base path in my loopback application by using this.basePath('/api') that aren't reflected in /openapi.json.

Server url like below(https://127.0.0.1/)
https://user-images.githubusercontent.com/42985749/60344156-e0099280-9983-11e9-838b-4a1eb45889be.png

https://user-images.githubusercontent.com/42985749/60344373-5efecb00-9984-11e9-8ca5-dd7d11ff0c98.png

I need servers url like(https://127.0.0.1/api/)
https://user-images.githubusercontent.com/1140553/60504408-d5bc0100-9cc1-11e9-9fc0-25f1d1ef8040.png

@raymondfeng
Copy link
Contributor

The feature should have been released for a while now.

@AnanthGopal
Copy link

thank you @raymondfeng . Eagerly i am waiting for this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants