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

[CarmenPopoviciu]

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 evanw/esbuild#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)
  • 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 afaik

[changeset-bot]

🦋 Changeset detected

Latest commit: 50aa43c

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

[github-actions]

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.

---

wrangler@3.62.0 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.

[CarmenPopoviciu]

soooo...turns out, while testing the _worker.js work, that there is at least one corner case where this seems not to be true.

Say our Function math-is-fun.ts imports ../math/add.ts and calls add(). So far so good. Now imagine we delete the contents of add.ts but leave everything else as it is. We'll get a compile warning saying:

Import "add" will always be undefined because the file "../math/add.ts" has no exports

which is correct. At this point, IMHO, add.ts is technically still a dependency of math-is-fun.ts (though maybe we can argue abt that later), so my expectation would be that if we were to undo the changes we made in add.ts, that would trigger a rebuild. This works as expected if esbuild was in watch mode, but it does not trigger anything with the current implementation. The reason for that is that the metafile returned by esbuild does not list add.ts as an input to the entrypoint, which means we won't return it as part of bundle.dependecies 😕. The returned metafile BTW, looks exactly the same in both esbuild/chokidar watch modes. So, it's either that esbuild watches more files than what it returns in the metafile, or we're making some wrong assumptions when we process that metafile to return bundle.dependencies, or there is a plugin somewhere that strips out empty imports? (meaning of 0kb in size).

Whatever it is, the finding is a tad worrying...cause it makes me wonder...what other corner cases I didn't think of yet, would result in a similar behaviour 🫠

it's never easy is it 😅 🍦 (i-scream 😉 )

[CarmenPopoviciu]

raised here: #6182

[andyjessop]

LGTM 🔥

[CarmenPopoviciu]

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