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

[wrangler] fix: allow wrangler pages dev sessions to be reloaded #4054

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Sep 28, 2023

Fixes #4008, #4017, remix-run/remix#7510

What this PR solves / how to test:

#3951 introduced a change that closed the bootstrapper's IPC channel when Wrangler exited. Whenever wrangler (pages) dev reloads, an event is sent on this channel to indicate the dev server is ready. Unfortunately, wrangler pages dev's handler didn't wait for the dev session to exit before resolving its returned promise, meaning the channel was closed as soon as the dev server was ready, not when Wrangler exited. This meant that ready events sent from reloads resulted in an ERR_IPC_CHANNEL_CLOSED that crashed Wrangler. :(

This PR updates the code to only resolve the returned promise once the dev session exits, ensuring the channel stays open.

To test, run npm run dev in fixtures/pages-functions-app, wait for the dev server to start, then update a file. Before this change, this would crash. Afterwards, it should reload the dev server with the new script.

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

N/A

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.

#3951 introduced a change that closed the bootstrapper's IPC channel
when Wrangler exited. Whenever `wrangler (pages) dev` reloads, an
event is sent on this channel to indicate the dev server is ready.
Unfortunately, `wrangler pages dev`'s handler doesn't wait for the
dev session to exit before resolving its returned promise, meaning
the channel is closed as soon as the dev server is ready, not when
Wrangler exits. This means that ready events sent from reloads result
in an `ERR_IPC_CHANNEL_CLOSED` that crashes Wrangler. :(
@mrbbot mrbbot requested review from a team as code owners September 28, 2023 16:01
@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2023

🦋 Changeset detected

Latest commit: 5e99f2f

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

@github-actions
Copy link
Contributor

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/6341382186/npm-package-wrangler-4054

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

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

Or you can use npx with this latest build directly:

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

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


wrangler@3.10.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare 3.20230922.0 3.20230922.0
workerd 1.20230922.0 1.20230922.0
workerd --version 1.20230922.0 2023-09-22

|

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

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #4054 (5e99f2f) into main (0d4a4ca) will increase coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4054      +/-   ##
==========================================
+ Coverage   75.03%   75.06%   +0.02%     
==========================================
  Files         216      216              
  Lines       12029    12029              
  Branches     3116     3116              
==========================================
+ Hits         9026     9029       +3     
+ Misses       3003     3000       -3     
Files Coverage Δ
packages/wrangler/src/pages/dev.ts 16.22% <0.00%> (ø)

... and 2 files with indirect coverage changes

@mrbbot mrbbot merged commit f8c52b9 into main Sep 28, 2023
16 checks passed
@mrbbot mrbbot deleted the bcoll/pages-dev-wait-for-exit branch September 28, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants