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: wait for port to be available before creating a dev server #332

Merged
merged 1 commit into from
Jan 29, 2022

Conversation

threepointone
Copy link
Contributor

When we run wrangler dev, we start a server on a port (defaulting to 8787). We do this separately for both local and edge modes. However, when switching between the two with the l hotkey, we don't 'wait' for the previous server to stop before starting the next one. This can crash the process, and we don't want that (of course). So we introduce a helper function waitForPortToBeAvailable() that waits for a port to be available before returning. This is used in both the local and edge modes, and prevents the bug right now, where switching between edge - local - edge crashes the process.

(This isn't a complete fix, and we can still cause errors by very rapidly switching between the two modes. A proper long term fix for the future would probably be to hoist the proxy server hook above the <Remote/> and <Local/> components, and use a single instance throughout. But that requires a deeper refactor, and isn't critical at the moment.)

When we run `wrangler dev`, we start a server on a port (defaulting to 8787). We do this separately for both local and edge modes. However, when switching between the two with the `l` hotkey, we don't 'wait' for the previous server to stop before starting the next one. This can crash the process, and we don't want that (of course). So we introduce a helper function `waitForPortToBeAvailable()` that waits for a port to be available before returning. This is used in both the local and edge modes, and prevents the bug right now, where switching between edge - local - edge crashes the process.

(This isn't a complete fix, and we can still cause errors by very rapidly switching between the two modes. A proper long term fix for the future would probably be to hoist the proxy server hook above the `<Remote/>` and `<Local/>` components, and use a single instance throughout. But that requires a deeper refactor, and isn't critical at the moment.)
@changeset-bot
Copy link

changeset-bot bot commented Jan 28, 2022

🦋 Changeset detected

Latest commit: 6af0dc6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

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

@petebacondarwin
Copy link
Contributor

Would it be worth considering using a different port for local mode?

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Can you create a TODO or an issue to track the proper fix/refactor for the future?

@threepointone
Copy link
Contributor Author

I considered using a different port for local mode, but that would make development annoying, you'd have to open a fresh tab, whereas here you can just refresh the existing tab.

@threepointone threepointone merged commit a2155c1 into main Jan 29, 2022
@threepointone threepointone deleted the wait-for-port branch January 29, 2022 16:10
@github-actions github-actions bot mentioned this pull request Jan 29, 2022
@petebacondarwin
Copy link
Contributor

I considered using a different port for local mode, but that would make development annoying, you'd have to open a fresh tab, whereas here you can just refresh the existing tab.

Yeah that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants