-
Notifications
You must be signed in to change notification settings - Fork 778
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: add support for --ip
and config.dev.ip
in the dev command
#647
Conversation
🦋 Changeset detectedLatest commit: 0cb838b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2012791280/npm-package-wrangler-647 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/647/npm-package-wrangler-647 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2012791280/npm-package-wrangler-647 dev path/to/script.js |
7c736bd
to
7a890bc
Compare
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.
🐿
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 think we should also reflect this here https://github.com/cloudflare/wrangler2/blob/61aea3052f90dc7a05f77dd2d60e8b32af143a83/packages/wrangler/src/dev/dev.tsx#L312
proxyServer.listen(port); | ||
console.log(`⬣ Listening at ${localProtocol}://localhost:${port}`); | ||
proxyServer.listen(port, ip); | ||
console.log(`⬣ Listening at ${localProtocol}://${ip}:${port}`); |
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.
could we show the ip here only if it isn't 127.0.0.1?
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.
Strictly localhost
only refers to 127.0.0.1
if that is what is in the/etc/hosts
(on OS/X) file.
Instead, how about we change the default for ip
to be localhost
rather than 127.0.01
, which would see more appropriate.
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.
Yeah that sounds good
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.
lemme stamp this after the openInBrowser change
7a890bc
to
e077e40
Compare
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.
Preemptive stamp, feel free to take a call on the localhost thing.
Note that this change modifies the default listening address to `localhost`, which is different to `127.0.0.1`, which is what Wrangler 1 does. For most developers this will make no observable difference, since the default host mapping in most OSes from `localhost` to `127.0.0.1`. Resolves [cloudflare#584](cloudflare#584)
e077e40
to
0cb838b
Compare
Resolves #584