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): Watch for external dependencies changes in pages dev #6022

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

CarmenPopoviciu
Copy link
Contributor

@CarmenPopoviciu CarmenPopoviciu commented Jun 12, 2024

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

Copy link

changeset-bot bot commented Jun 12, 2024

🦋 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

Copy link
Contributor

github-actions bot commented Jun 12, 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/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 CarmenPopoviciu force-pushed the carmen/dev-live-reload branch 2 times, most recently from f4e5889 to 5777491 Compare June 17, 2024 16:43
.changeset/gorgeous-oranges-brake.md Outdated Show resolved Hide resolved
packages/wrangler/e2e/pages-dev.test.ts Outdated Show resolved Hide resolved
packages/wrangler/e2e/pages-dev.test.ts Outdated Show resolved Hide resolved
packages/wrangler/e2e/pages-dev.test.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 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/functions/buildWorker.ts Outdated Show resolved Hide resolved
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload branch 2 times, most recently from aae7652 to 5abcd1f Compare June 28, 2024 11:16

/*
*`bundle.dependencies` and `bundle.modules` will always contain the
* latest dependency list of the current bundle. If we are currently
Copy link
Contributor Author

@CarmenPopoviciu CarmenPopoviciu Jun 28, 2024

Choose a reason for hiding this comment

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

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 😉 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

raised here: #6182

@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload branch 6 times, most recently from eeacb74 to 05d1e4b Compare July 1, 2024 11:32
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload branch 2 times, most recently from c91197e to 4b1accd Compare July 1, 2024 13: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.

LGTM 🔥

packages/wrangler/src/pages/dev.ts Show resolved Hide resolved
packages/wrangler/src/pages/dev.ts Show resolved Hide resolved
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/dev-live-reload branch 2 times, most recently from ca9fc3a to 36e1aaa Compare July 1, 2024 15:13
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 commit fixes all these things, plus adds some extra
polish to the `pages dev` watch mode experience.

Fixes #3840
@CarmenPopoviciu CarmenPopoviciu merged commit 7951815 into main Jul 2, 2024
21 checks passed
@CarmenPopoviciu CarmenPopoviciu deleted the carmen/dev-live-reload branch July 2, 2024 11:14
@CarmenPopoviciu
Copy link
Contributor Author

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

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
Archived in project
Development

Successfully merging this pull request may close these issues.

🐛 BUG: wrangler pages dev doesn't recompile worker when imported dependency changes
4 participants