-
Notifications
You must be signed in to change notification settings - Fork 568
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
[wrangler] Make wrangler dev --experimental-local
the default 🎉
#3224
Conversation
🦋 Changeset detectedLatest commit: 78720c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
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/5003291085/npm-package-wrangler-3224 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/3224/npm-package-wrangler-3224 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5003291085/npm-package-wrangler-3224 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5003291085/npm-package-cloudflare-pages-shared-3224 Note that these links will no longer work once the GitHub Actions artifact expires. |
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.
Looks great (for a mammoth PR 😆 ), have some questions before approving
// Our inspector proxy server will be binding to the result of | ||
// `getInspectorPort`. If we attempted to bind workerd to the same inspector | ||
// port, we'd get a port already in use error. Therefore, generate a new port | ||
// for our runtime to bind its inspector service to. | ||
const getRuntimeInspectorPort = memoizeGetPort(); |
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.
Are we certain these Node processes are getting cleaned up after use?
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.
Pretty sure. You can't have two processes listening on the same port, so we definitely need separate ports for Wrangler's inspector proxy server and the runtime's inspector server.
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.
My concern is creating tons of zombie Node processes holding onto more and more ports.
wrangler dev --experimental-local
the default 🎉wrangler dev --experimental-local
the default 🎉
Codecov Report
@@ Coverage Diff @@
## v3 #3224 +/- ##
==========================================
+ Coverage 74.55% 75.29% +0.73%
==========================================
Files 179 180 +1
Lines 10969 10895 -74
Branches 2922 2851 -71
==========================================
+ Hits 8178 8203 +25
+ Misses 2791 2692 -99
|
Won't block, questions and concerns resolved.
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.
general vibe check for the code from Pages looks good. I want to actually test it a bunch tonight still though.
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.
the rare double approval. gave the Pages functionality a whirl. Looks good to me in practice too ✅✅
package-lock.json
Outdated
@@ -94902,7 +92549,7 @@ | |||
"version": "0.1.0" | |||
}, | |||
"yoga-layout": { | |||
"version": "file:../../vendor/yoga-layout-2.0.0-beta.1.tgz", | |||
"version": "file:..\\..\\vendor\\yoga-layout-2.0.0-beta.1.tgz", |
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.
Refresh by running npm i
on unix machine
Consider adding |
...and make `--local` the default and add `--remote` flag
This reverts commit 13abe44.
26bab6b
to
78720c2
Compare
) * Use Miniflare 3 for `--local` and deprecate `--experimental-local` ...and make `--local` the default and add `--remote` flag * Revert "fix: c3 arguments" This reverts commit 13abe44.
Closes #2821
Closes DEVX-605
Closes DEVX-625
Closes DEVX-626
Closes DEVX-634
What this PR solves / how to test:
This PR...
--local
implementation powered by Miniflare 2--experimental-local
implementation powered by Miniflare 3 to--local
--local
the default--local
/--experimental-local
flags--remote
flag to restore the old behaviourPreviously, the
--experimental-local
implementation was built on-top of the previous--local
one. As we're removing--local
, this meant there was a lot of untangling to do. We're planning some architectural improvements towrangler dev
soon. To see what these might look like, the new Miniflare 3 implementation has been implemented using the proposed approach, and integrated with the existing system to demonstrate incremental adoptability.This PR also adds a few missing features/fixes to the
--experimental-local
implementation:In terms of other breaking changes, remote KV storage in local mode has been removed as this is incompatible with Miniflare's new storage system. This was an experimental feature we'll need to rethink. This new storage system also changed the persistence format, invalidating all previously persisted data. To avoid loading invalid data, the default persistence directory has been changed.
Still TODO (wanted to get this PR up so we can start testing the pre-release):
wrangler dev
is now using local mode on first runAssociated docs issue(s)/PR(s):
Author has included the following, where applicable:
Reviewer is to perform the following, as applicable: