-
Notifications
You must be signed in to change notification settings - Fork 94
feat(world-local): add baseUrl config and improve port handling #260
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
feat(world-local): add baseUrl config and improve port handling #260
Conversation
🦋 Changeset detectedLatest commit: 94d5e51 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@aryasaatvik is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
…ion and config Signed-off-by: Saatvik Arya <aryasaatvik@gmail.com>
|
since #233 was merged, could you please rebase/merge this PR on main? this or includes a number of commits from @adriandlam |
57fbd63 to
6c377ff
Compare
I, Saatvik Arya <aryasaatvik@gmail.com>, hereby add my Signed-off-by to this commit: 195a7ae I, Saatvik Arya <aryasaatvik@gmail.com>, hereby add my Signed-off-by to this commit: 414aa4c I, Saatvik Arya <aryasaatvik@gmail.com>, hereby add my Signed-off-by to this commit: 7b81781 Signed-off-by: Saatvik Arya <aryasaatvik@gmail.com>
- Change port check from truthiness to `typeof === 'number'` - Port 0 (OS-assigned port) now works correctly instead of being skipped - Null values properly fall through to auto-detection - Add tests for port 0 and null edge cases Signed-off-by: Saatvik Arya <aryasaatvik@gmail.com>
The createEmbeddedWorld API signature change is breaking: - Before: createEmbeddedWorld(dataDir?, port?) - After: createEmbeddedWorld(args?: Partial<Config>) Signed-off-by: Saatvik Arya <aryasaatvik@gmail.com>
These were bugs introduced and fixed within the same PR, not pre-existing bugs, so they shouldn't be listed as fixes. Signed-off-by: Saatvik Arya <aryasaatvik@gmail.com>
Automatic port detection already existed in main via getPort(). This PR only adds baseUrl config and port 0 support. Signed-off-by: Saatvik Arya <aryasaatvik@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
VaguelySerious
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.
Thanks for contributing! The code looks good to me overall and I like the change.
I ran unit tests on this PR and it looks like there might be some e2e test failures - could you take a look? Your commits don't trigger tests, but you can test e2e locally by running e2e tests in packages/core against e.g. the nextjs-turbopack workbench app running in dev mode. See the github tests.yml file on how that's set up.
Once tests are green, this would be good to merge
| describe('edge cases', () => { | ||
| it('should handle empty config object', async () => { | ||
| const { getPort } = await import('@workflow/utils/get-port'); | ||
| vi.mocked(getPort).mockResolvedValue(undefined); |
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.
| vi.mocked(getPort).mockResolvedValue(undefined); | |
| vi.mocked(getPort).mockResolvedValue({}); |
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.
fixed the issue. it was overriding defaults with undefined. getPort returns undefined instead of an empty object
- Updated createEmbeddedWorld to filter out undefined values from args before merging with the default config. - This change ensures that only defined arguments are considered, enhancing the function's robustness. Signed-off-by: Saatvik Arya <aryasaatvik@gmail.com>
|
Looks great, thanks again! |
Add baseUrl configuration and improve port handling
This PR adds baseUrl configuration support to enable HTTPS and custom hostnames in local development, while maintaining automatic port detection as a fallback.
Key Features
BaseUrl override:
baseUrlconfig option orWORKFLOW_EMBEDDED_BASE_URLenv varnext dev --experimental-https)https://local.example.com:3000)Port resolution priority:
config.baseUrl(full override)config.port(explicit port)@workflow/utils/get-portBreaking Changes
API signature updated:
Closes #233
Closes #245