-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(rest): make sure basePath is included in RestServer.url #2560
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
Conversation
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
});6d178fc to
ed31908
Compare
|
@bajtos I added as a way to access |
packages/rest/src/__tests__/integration/rest.application.integration.ts
Outdated
Show resolved
Hide resolved
ed31908 to
489cc3c
Compare
489cc3c to
0f93da4
Compare
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks much better now! Let's do few more improvements though.
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
0f93da4 to
f8a7248
Compare
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
RestServer.url does not reflect
basePath. The problem can be reproduced by adding basePath: '/api' to examples/todo/index.js and runnpm 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 testpasses on your machinepackages/cliwere updatedexamples/*were updated