workers-sdk icon indicating copy to clipboard operation
workers-sdk copied to clipboard

fix(wrangler): Watch for external dependencies changes in `pages dev`

Open CarmenPopoviciu opened this issue 1 year ago • 2 comments

What this PR solves / how to test

The watch mode in pages dev for Pages Functions projects is currently partially broken, as it only watches for file system changes in the /functions directory, but not for changes in any of the Functions' dependencies. This means that given a Pages Function math-is-fun.ts, defined as follows:

import { ADD } from "../math/add";

export async function onRequest() {
  return new Response(`${ADD} is fun!`);
}

pages dev will reload for any changes in math-is-fun.ts itself, but not for any changes in math/add.ts, 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 PR fixes all these things, plus adds some extra polish to the pages dev watch mode experience.

Fixes #3840

Solutions we considered

Pages Functions projects cannot rely on esbuild's watch mode alone. That's because the watch mode that's built into esbuild is designed specifically for only triggering a rebuild when esbuild's build inputs are changed (see https://github.com/evanw/esbuild/issues/3705). With Functions, we would actually want to trigger a rebuild every time a new file is added to the "/functions" directory.

One solution would be to use an esbuild plugin, and "teach" esbuild to watch the "/functions" directory. But it's more complicated than that. When we build Functions, one of the steps is to generate the routes based on the file tree (see generateConfigFileFromTree). These routes are then injected into the esbuild entrypoint (see templates/pages-template-worker.ts). Delegating the "/functions" dir watching to an esbuild plugin, would mean delegating the routes generation to that plugin as well. This gets very hairy very quickly.

Another solution, is to use a combination of dependencies watching, via esbuild, and file system watching, via chokidar. The downside of this approach is that a lot of syncing between the two watch modes must be put in place, in order to avoid triggering building Functions multiple times over one single change (like for example renaming file that's part of the dependency graph).

Another solution, which is the one we opted for here, is to delegate file watching entirely to a file system watcher, chokidar in this case. While not entirely downside-free

  • we still rely on esbuild to provide us with the dependency graph
  • we need some logic in place to pre-process and filter the dependencies we pass to chokidar
  • we need to keep track of which dependencies are being watched

this solution keeps all things watch mode in one place, makes things easier to read, reason about and maintain, separates Pages <-> esbuild concerns better, and gives all the flexibility we need.

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:

  • add/change/delete Function file in functions
  • add/change/delete Function file in functions/<subdir>/**/<subdir>
  • add/change/delete _middleware.ts in functions
  • add/change/delete _middleware.ts in functions/<subdir>/**/<subdir>
  • change in external dependency (eg import { ADD } from "../<dir>/**/<dir>/add")
  • change in external module (eg import html from "../<dir>/**/<dir>/my-html.html")
  • bulk file add/delete
  • bulk directory add/delete

Author has addressed the following

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

CarmenPopoviciu avatar Jun 12 '24 17:06 CarmenPopoviciu

🦋 Changeset detected

Latest commit: 50aa43c459e456d7d752abe6e3f005f61121c7bc

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

changeset-bot[bot] avatar Jun 12 '24 17:06 changeset-bot[bot]

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/9759645322/npm-package-wrangler-6022

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

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

Or you can use npx with this latest build directly:

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

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.20240620.0
workerd 1.20240620.1 1.20240620.1
workerd --version 1.20240620.1 2024-06-20

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

github-actions[bot] avatar Jun 12 '24 17:06 github-actions[bot]

thank you everyone for reviewing! It is much much appreciated <3. Merged! 🎉

CarmenPopoviciu avatar Jul 02 '24 11:07 CarmenPopoviciu