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): make sure basePath is included in RestServer.url #2560

Merged
merged 1 commit into from Mar 25, 2019

Conversation

Projects
None yet
3 participants
@raymondfeng
Copy link
Member

raymondfeng commented Mar 8, 2019

RestServer.url does not reflect basePath. The problem can be reproduced by adding basePath: '/api' to examples/todo/index.js and run npm build:

The url is printed as http://127.0.0.1:3000, which returns 404. I think the url should include the basePath, i.e., 'http://127.0.0.1:3000/api'.

See #2554 (comment)

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

@raymondfeng raymondfeng requested a review from bajtos as a code owner Mar 8, 2019

@raymondfeng raymondfeng referenced this pull request Mar 8, 2019

Open

Openapi and api explorer to honor basePath #2554

4 of 7 tasks complete

@bajtos bajtos added bug REST labels Mar 12, 2019

@bajtos bajtos requested a review from hacksparrow Mar 12, 2019

@bajtos
Copy link
Member

bajtos left a comment

I wish we had a TSDoc comment for the url property to make it clear what is the expected value. The property was added by @hacksparrow in 18b3408. @hacksparrow do you remember what was your intention for adding this property and whether it's ok to include basePath?

@raymondfeng please add a new test (or more) to directly and explicitly verify the behavior of url property when basePath is set. See the test added by 18b3408 to packages/rest/test/integration/rest.server.integration.ts, I think new tests should be added to the same place.

Personally, I'd create a new describe block to group all url-related tests.

describe('RestServer (integration)', () => {
  describe('url', () => {
    it('provides URL of the server'); // the existing test
    it('includes basePath when set'); // a new test to add
  });
  // no changes in other tests
});
Show resolved Hide resolved packages/rest/src/rest.server.ts

@raymondfeng raymondfeng force-pushed the fix-rest-server-url branch from 6d178fc to ed31908 Mar 12, 2019

@hacksparrow

This comment has been minimized.

Copy link
Member

hacksparrow commented Mar 13, 2019

@bajtos I added as a way to access this._httpServer.url. Don't remember adding any functionality based around it.

@raymondfeng raymondfeng force-pushed the fix-rest-server-url branch from ed31908 to 489cc3c Mar 19, 2019

@raymondfeng raymondfeng requested a review from bajtos Mar 19, 2019

@raymondfeng raymondfeng force-pushed the fix-rest-server-url branch from 489cc3c to 0f93da4 Mar 21, 2019

@raymondfeng raymondfeng requested a review from hacksparrow Mar 22, 2019

@bajtos
Copy link
Member

bajtos left a comment

The change looks much better now! Let's do few more improvements though.

Show resolved Hide resolved packages/rest/src/__tests__/integration/rest.server.integration.ts Outdated
Show resolved Hide resolved packages/rest/src/__tests__/integration/rest.server.integration.ts Outdated
Show resolved Hide resolved packages/rest/src/rest.server.ts Outdated
Show resolved Hide resolved packages/rest/src/rest.server.ts Outdated
Show resolved Hide resolved packages/testlab/src/client.ts Outdated

@raymondfeng raymondfeng force-pushed the fix-rest-server-url branch from 0f93da4 to f8a7248 Mar 22, 2019

@raymondfeng raymondfeng requested a review from bajtos Mar 22, 2019

@bajtos

bajtos approved these changes Mar 25, 2019

Copy link
Member

bajtos left a comment

馃憦

@raymondfeng raymondfeng merged commit 705bce4 into master Mar 25, 2019

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.001%) to 90.965%
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

@raymondfeng raymondfeng deleted the fix-rest-server-url branch Mar 25, 2019

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