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

Actually respect port and host option #424

Merged
merged 2 commits into from
Nov 7, 2022
Merged

Actually respect port and host option #424

merged 2 commits into from
Nov 7, 2022

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Nov 7, 2022

Previously, port was being passed through get-port before being used. This meant that if Miniflare was disposed and then immediately created again (as opposed to using setOptions), it couldn't use the same port, as get-port deliberately doesn't allocate recently allocated ports.

Notably, Wrangler does exactly this: re-create Miniflare instances. Ideally, we should be using setOptions there, but this at least means the port stays the same between reloads.

This PR also passes the host option to workerd, now that we're not putting an HTTP proxy in front of workerd in Miniflare.

Previously, `port` was being passed through `get-port` before being
used. This meant that if `Miniflare` was disposed and then
immediately created again (as opposed to using `setOptions`), it
couldn't use the same `port`, as `get-port` deliberately doesn't
allocate recently allocated ports.

Notably, Wrangler does exactly this: re-create `Miniflare` instances.
Ideally, we should be using `setOptions` there, but this at least
means the port stays the same between reloads.

This PR also passes the `host` option to `workerd`, now that we're
not putting an HTTP proxy in front of `workerd` in Miniflare.
@mrbbot mrbbot added the tre Relating to Miniflare 3 label Nov 7, 2022
@changeset-bot
Copy link

changeset-bot bot commented Nov 7, 2022

⚠️ No Changeset found

Latest commit: 1327f2a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Ctrl/CMD-cliking on this URL in VSCode included the `!` in the URL,
opening the wrong URL in browser.
Copy link
Contributor

@penalosa penalosa left a comment

Choose a reason for hiding this comment

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

This takes the port selection out of getPort, which means if the core specified port is in use, Miniflare will now fail rather than recovering with a different port. Is there some way to get around that? (perhaps catching the requisite error from workerd?)

// TODO: respect entry `host` option
this.#runtime = new this.#runtimeConstructor(entryPort, loopbackPort);
const host = this.#sharedOpts.core.host ?? "127.0.0.1";
const port = this.#sharedOpts.core.port ?? (await getPort({ port: 8787 }));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will now fail if core.port is in use, rather than falling back to 8787

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think this is nicer behaviour. If the user is explicitly setting the port option, we should fail if we can't bind to that port. Previously, this option was acting more like preferredPort which I don't think is right. If users want this behaviour, they could call getPort themselves before calling new Miniflare().

@mrbbot mrbbot merged commit c40fe41 into tre Nov 7, 2022
@mrbbot mrbbot deleted the tre-respect-host-port branch November 7, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tre Relating to Miniflare 3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants