-
Notifications
You must be signed in to change notification settings - Fork 710
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
Allow --experimental-local
to cleanly exit on x
/CTRL-C
#2400
Conversation
In the bootstrapper script `bin/wrangler.js`, we open an IPC channel, so IPC messages from the Wrangler process are propagated through the bootstrapper. Normally, Node's SIGINT handler would close this for us, but interactive mode enables raw mode on stdin which disables the built-in handler. This PR calls `process.disconnect()` to close this channel when we press `x` or CTRL-C, ensuring no open handles, and allowing for a clean exit. Closes #2387
🦋 Changeset detectedLatest commit: b3540d5 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.devprod.cloudflare.dev/runs/3703857564/npm-package-wrangler-2400 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/prs/2400/npm-package-wrangler-2400 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/runs/3703857564/npm-package-wrangler-2400 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/runs/3703857564/npm-package-cloudflare-pages-shared-2400 |
Codecov Report
@@ Coverage Diff @@
## main #2400 +/- ##
==========================================
+ Coverage 70.71% 71.51% +0.80%
==========================================
Files 154 155 +1
Lines 9758 9697 -61
Branches 2551 2537 -14
==========================================
+ Hits 6900 6935 +35
+ Misses 2858 2762 -96
|
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.
Accepting that a proper test for this will be added in #2365 - LGTM.
Nice comments.
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.
Hmm... actually, I just tried the preview build of this:
pwd
> wrangler2/fixtures/worker-app
npx https://prerelease-registry.devprod.cloudflare.dev/runs/3685399221/npm-package-wrangler-2400 dev --experimental-local
And I still had to add the extra Ctrl-C after pressing Q.
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.
Sorry, I was falling foul of the old wrangler
uses the local install concern.
After deleting node_modules and locally installing the version from this PR - it all looks 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.
LGTM! Is this a concern for unstable_dev
?
Good question. This is potentially a concern if |
What this PR solves / how to test:
In the bootstrapper script
bin/wrangler.js
, we open an IPC channel, so IPC messages from the Wrangler process are propagated through the bootstrapper. Normally, Node's SIGINT handler would close this for us, but interactive mode enables raw mode on stdin which disables the built-in handler. This PR callsprocess.disconnect()
to close this channel when we pressx
or CTRL-C, ensuring no open handles, and allowing for a clean exit.This PR can be tested by running
wrangler dev --experimental-local
on a simple Worker script, then pressing,x
,q
and/orCTRL-C
.Associated docs issues/PR:
N/A
Author has included the following, where applicable:
Tests(will make sure this is tested as part of Implementwrangler dev --(experimental-)local
E2E tests #2365)Reviewer has performed the following, where applicable:
Fixes #2387