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

fix(wrangler): Fix pages dev watch mode [_worker.js] #6150

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

CarmenPopoviciu
Copy link
Contributor

@CarmenPopoviciu CarmenPopoviciu commented Jun 26, 2024

What this PR solves / how to test

The watch mode in pages dev for Advanced Mode projects is currently partially broken, as it only watches for changes in the _worker.js file, but not for changes in any of its imported dependencies. This means that given the following _worker.js file

import { graham } from "./graham-the-dog";
export default {
    fetch(request, env) {
	return new Response(graham)
    }
}

pages dev will reload for any changes in the _worker.js file itself, but not for any changes in graham-the-dog.js, which is its dependency.

Similarly, pages dev will not reload for any changes in non-JS module imports, such as wasm/html/binary module imports.

This commit addresses all the aforementioned issues.

Fixes #4824

Solutions we considered

Our original solution was implemented using esbuild's watch mode (see dd6cd19), which basically meant just enabling the watch flag in buildRawWorker(). Turned out though (via failed test in CI), that this solution introduced a regression for a frameworks corner case...the deletion of _worker.js itself (please see #4877 and #3886).

When used in the context of frameworks, the assets directory served by pages dev gets rebuilt (removed + added back with updated assets) by a separate process, external to wrangler (I'm no expert here on the exact details, but I assume this more or less to be the case). This affects key Pages files, such as _worker.js and _routes.json, which get deleted/added back as part of that assets directory, which in turn, triggers a _worker.js rebuild. If the _worker.js re-build is triggered before the external process gets a chance to add the file back, pages dev will show a bunch of errors in the terminal, which, while accurate, seem to be confusing/annoying for our users. This was fixed by #4877 by completely turning off Worker rebuilds for chokidar "unlink` events.

Unfortunately, the behaviour for this particular use case is very hard to port to esbuild watch mode, since esbuild gives us very limited information wrt to what changed ....so here we are 😢

This lead us to the second implementation (see 5c8fcf0), which, similar to Functions, delegates the entirety of the watch mechanism to chokidar.

How we tested the changes

This PR comes with e2e tests, but in addition to them, we've manually tested these changes in the following scenarios:

  • change/remove/add _worker.js
  • change in external dependency (eg import { ADD } from "..//**//add")
  • change in external module (eg import html from "..//**//my-html.html")
  • adding/removing external dependencies
  • errors in _worker.js
  • errors in dependencies

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Included
    • Not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required / Maybe required
    • Not required because:
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Not necessary because: watch mode is not documented

@CarmenPopoviciu CarmenPopoviciu requested review from a team as code owners June 26, 2024 02:09
Copy link

changeset-bot bot commented Jun 26, 2024

🦋 Changeset detected

Latest commit: de6fbc2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@cloudflare/vitest-pool-workers 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

@workers-devprod workers-devprod added the e2e Run e2e tests on a PR label Jun 26, 2024
Copy link
Contributor

github-actions bot commented Jun 26, 2024

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/9796131036/npm-package-wrangler-6150

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9796131036/npm-package-wrangler-6150 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9796131036/npm-package-create-cloudflare-6150 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9796131036/npm-package-cloudflare-kv-asset-handler-6150
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9796131036/npm-package-miniflare-6150
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9796131036/npm-package-cloudflare-pages-shared-6150
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9796131036/npm-package-cloudflare-vitest-pool-workers-6150

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


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240701.0
workerd 1.20240701.0 1.20240701.0
workerd --version 1.20240701.0 2024-07-01

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

@CarmenPopoviciu CarmenPopoviciu marked this pull request as draft June 26, 2024 08:37
Copy link
Contributor

@andyjessop andyjessop left a comment

Choose a reason for hiding this comment

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

Looks great! Just some minor comments, but nothing blocking.

packages/wrangler/src/pages/functions/buildWorker.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pages/dev.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pages/functions/buildWorker.ts Outdated Show resolved Hide resolved
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload-workerjs branch 5 times, most recently from 00aef57 to 4cb8715 Compare July 2, 2024 14:35
@CarmenPopoviciu CarmenPopoviciu marked this pull request as ready for review July 2, 2024 14:35
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload-workerjs branch from 4cb8715 to d4e1c8c Compare July 2, 2024 14:42
packages/wrangler/src/pages/dev.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pages/dev.ts Show resolved Hide resolved
packages/wrangler/src/pages/dev.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pages/dev.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pages/dev.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pages/dev.ts Outdated Show resolved Hide resolved
@andyjessop andyjessop self-requested a review July 2, 2024 14:56
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload-workerjs branch from d4e1c8c to 536836f Compare July 3, 2024 12:07
@penalosa
Copy link
Contributor

penalosa commented Jul 3, 2024

Approved pending #6150 (comment), which I'm likely misunderstanding

@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload-workerjs branch 3 times, most recently from d360b9f to e3b1f9b Compare July 4, 2024 13:49
The watch mode in `pages dev` for Advanced Mode projects
is currently partially broken, as it only watches for
changes in the `_worker.js` file, but not for changes in
any of its imported dependencies. This means that given
the following `_worker.js` file

```
import { graham } from "./graham-the-dog";
export default {
    fetch(request, env) {
	return new Response(graham)
    }
}
```

`pages dev` will reload for any changes in the `_worker.js`
file itself, but not for any changes in `graham-the-dog.js`,
which is its dependency.

Similarly, `pages dev` will not reload for any changes in
non-JS module imports, such as wasm/html/binary module
imports.

This commit addresses all the aforementioned issues.

Fixes #4824
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload-workerjs branch from e3b1f9b to de6fbc2 Compare July 4, 2024 14:31
@penalosa penalosa merged commit d993409 into main Jul 4, 2024
20 checks passed
@penalosa penalosa deleted the carmen/dev-live-reload-workerjs branch July 4, 2024 15:06
@workers-devprod workers-devprod mentioned this pull request Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
Status: Done
4 participants