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

Default local dev server to * #3733

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Default local dev server to * #3733

merged 1 commit into from
Sep 20, 2023

Conversation

jspspike
Copy link
Contributor

@jspspike jspspike commented Aug 10, 2023

Fixes #3732

Depends on cloudflare/miniflare#650 being merged and released

Associated docs issue(s)/PR(s):

cloudflare/cloudflare-docs#10320

Author has included the following, where applicable:

Reviewer is to perform the following, as applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@jspspike jspspike requested a review from a team as a code owner August 10, 2023 18:17
@changeset-bot
Copy link

changeset-bot bot commented Aug 10, 2023

🦋 Changeset detected

Latest commit: a09a4fa

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

@jspspike jspspike requested review from a team as code owners August 10, 2023 18:21
@jspspike jspspike force-pushed the jspspike/localhost branch 2 times, most recently from a7977b3 to 10993a0 Compare August 10, 2023 18:28
@jspspike jspspike marked this pull request as draft August 10, 2023 18:29
@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6251007994/npm-package-wrangler-3733

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6251007994/npm-package-wrangler-3733

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6251007994/npm-package-wrangler-3733 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6251007994/npm-package-cloudflare-pages-shared-3733

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.8.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare 3.20230918.0 3.20230918.0
workerd 1.20230904.0 1.20230904.0
workerd --version 1.20230904.0 2023-09-04

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@jspspike jspspike marked this pull request as ready for review September 7, 2023 15:00
@jspspike jspspike changed the title Default local dev server to :: Default local dev server to * Sep 7, 2023
@jspspike jspspike requested a review from mrbbot September 7, 2023 15:01
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #3733 (a09a4fa) into main (d8833ef) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3733      +/-   ##
==========================================
+ Coverage   74.85%   74.89%   +0.04%     
==========================================
  Files         196      196              
  Lines       11604    11604              
  Branches     3044     3044              
==========================================
+ Hits         8686     8691       +5     
+ Misses       2918     2913       -5     
Files Changed Coverage Δ
packages/wrangler/src/config/validation.ts 89.98% <100.00%> (ø)

... and 3 files with indirect coverage changes

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Functionally, looks good! Tested by running curl http://127.0.0.1:8787 and curl "http://[::1]:8787". 👍 One thing I've noticed though is there's quite a bit of logging. Every time I save my worker file I see...

[mf:inf] Updated and ready on http://*:8787 
[mf:inf] - http://127.0.0.1:8787
[mf:inf] - http://[::1]:8787
[mf:inf] - http://[fe80::1]:8787
[mf:inf] - http://127.0.2.2:8787
[mf:inf] - http://127.0.2.3:8787
[mf:inf] - http://[fe80::3cbe:4bff:fe5e:6eb1]:8787
[mf:inf] - http://[fe80::3cbe:4bff:fe5e:6eb0]:8787
[mf:inf] - http://[fe80::3cbe:4bff:fe5e:6eaf]:8787
[mf:inf] - http://[fe80::bcd0:74ff:fe37:b32c]:8787
[mf:inf] - http://[fe80::14b0:1478:434f:2494]:8787
[mf:inf] - http://192.168.0.46:8787
[mf:inf] - http://[fdba:a732:28d0:4944:8f3:842d:693b:8f20]:8787
[mf:inf] - http://[fe80::9817:7bff:feb2:549e]:8787
[mf:inf] - http://[fe80::9817:7bff:feb2:549e]:8787
[mf:inf] - http://[fe80::8b7c:2b8f:179:f497]:8787
[mf:inf] - http://[fe80::d633:1bf6:5607:ca25]:8787
[mf:inf] - http://[fe80::ce81:b1c:bd2c:69e]:8787
[mf:inf] - http://[fe80::3a5a:bc92:12a2:9a5b]:8787
[mf:inf] - http://[fe80::1cde:4059:8ab4:1d1e]:8787
[mf:inf] - http://192.168.0.163:8787
[mf:inf] - http://[fdba:a732:28d0:4944:1880:5946:afb2:2454]:8787

This is definitely accurate, but maybe we don't want to log all addresses every time. I'm not sure whether this should be changed in Miniflare or just Wrangler. If just Wrangler, we could do something sneaky in WranglerLog:

info(message: string) {

Maybe hide all info() logs starting with - after seeing a message starting with Updated and ready ? Or we could filter out the IPv6 addresses from the logs? AFAIK, most other JavaScript dev server CLIs don't include them.

@jspspike
Copy link
Contributor Author

Waiting on cloudflare/miniflare#686 to be merged to address above comment

@JacobMGEvans JacobMGEvans added blocked Blocked on other work miniflare Relating to Miniflare and removed blocked Blocked on other work labels Sep 15, 2023
@JacobMGEvans
Copy link
Contributor

cloudflare/miniflare#686 has been merged.

@jspspike
Copy link
Contributor Author

@JacobMGEvans I should have mentioned released and then miniflare version updated in wrangler. Which should happen soon!

@jspspike jspspike merged commit 7ba16cd into main Sep 20, 2023
16 checks passed
@jspspike jspspike deleted the jspspike/localhost branch September 20, 2023 16:35
irvinebroque added a commit that referenced this pull request Sep 22, 2023
@irvinebroque
Copy link
Contributor

For when we revisit — sounds like we would really benefit from some kind of remote dev integration / smoke test?

@mrbbot
Copy link
Contributor

mrbbot commented Sep 28, 2023

We should already have a basic one here...

it("can modify worker during dev session (remote)", async () => {
await runDevSession(workerPath, "--remote --ip 127.0.0.1", async (port) => {
const { text } = await retry(
(s) => s.status !== 200 || s.text === "",
async () => {
const r = await fetch(`http://127.0.0.1:${port}`);
return { text: await r.text(), status: r.status };
}
);
expect(text).toMatchInlineSnapshot('"Hello World!"');
await seed(workerPath, {
"src/index.ts": dedent`
export default {
fetch(request) {
return new Response("Updated Worker!")
}
}`,
});
// Give a bit of time for the change to propagate.
// Otherwise the process has a tendency to hang.
await setTimeout(5000);
const { text: text2 } = await retry(
(s) => s.status !== 200 || s.text === "Hello World!",
async () => {
const r = await fetch(`http://127.0.0.1:${port}`);
return { text: await r.text(), status: r.status };
}
);
expect(text2).toMatchInlineSnapshot('"Updated Worker!"');
});
});

Not sure why that didn't catch this 😕

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

Successfully merging this pull request may close these issues.

🐛 BUG: wrangler dev listens only on ipv4 localhost, not ipv6
5 participants