From 795181509a4735b16f426ac02873f04c208116c8 Mon Sep 17 00:00:00 2001 From: Carmen Popoviciu Date: Tue, 2 Jul 2024 13:14:11 +0200 Subject: [PATCH] fix(wrangler): Fix `pages dev` watch mode [Functions] (#6022) 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 --- .changeset/gorgeous-oranges-brake.md | 22 ++ packages/wrangler/e2e/pages-dev.test.ts | 324 +++++++++++------- packages/wrangler/src/pages/dev.ts | 166 ++++++++- .../src/pages/functions/buildWorker.ts | 16 +- packages/wrangler/src/pages/utils.ts | 19 + 5 files changed, 401 insertions(+), 146 deletions(-) create mode 100644 .changeset/gorgeous-oranges-brake.md diff --git a/.changeset/gorgeous-oranges-brake.md b/.changeset/gorgeous-oranges-brake.md new file mode 100644 index 000000000000..42226a5f6e60 --- /dev/null +++ b/.changeset/gorgeous-oranges-brake.md @@ -0,0 +1,22 @@ +--- +"wrangler": patch +--- + +fix: Fix `pages dev` watch mode [Functions] + +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. diff --git a/packages/wrangler/e2e/pages-dev.test.ts b/packages/wrangler/e2e/pages-dev.test.ts index 22da0d69cb0e..a7ca3eca2432 100644 --- a/packages/wrangler/e2e/pages-dev.test.ts +++ b/packages/wrangler/e2e/pages-dev.test.ts @@ -134,71 +134,6 @@ describe("pages dev", () => { ); }); - it("should modify worker during dev session (_worker)", async () => { - const helper = new WranglerE2ETestHelper(); - await helper.seed({ - "_worker.js": dedent` - export default { - fetch(request, env) { - return new Response("Hello World!") - } - }`, - }); - const port = await getPort(); - const worker = helper.runLongLived(`wrangler pages dev --port ${port} .`); - const { url } = await worker.waitForReady(); - - await expect( - fetch(url).then((r) => r.text()) - ).resolves.toMatchInlineSnapshot('"Hello World!"'); - - await helper.seed({ - "_worker.js": dedent` - export default { - fetch(request, env) { - return new Response("Updated Worker!") - } - }`, - }); - - await worker.waitForReload(); - - await expect( - fetch(url).then((r) => r.text()) - ).resolves.toMatchInlineSnapshot('"Updated Worker!"'); - }); - - it("should modify worker during dev session (Functions)", async () => { - const helper = new WranglerE2ETestHelper(); - const port = await getPort(); - const worker = helper.runLongLived(`wrangler pages dev --port ${port} .`); - - await helper.seed({ - "functions/_middleware.js": dedent` - export async function onRequest() { - return new Response("Hello World!") - }`, - }); - - const { url } = await worker.waitForReady(); - - await expect( - fetch(url).then((r) => r.text()) - ).resolves.toMatchInlineSnapshot('"Hello World!"'); - - await helper.seed({ - "functions/_middleware.js": dedent` - export async function onRequest() { - return new Response("Updated Worker!") - }`, - }); - - await worker.waitForReload(); - - await expect( - fetch(url).then((r) => r.text()) - ).resolves.toMatchInlineSnapshot('"Updated Worker!"'); - }); it("should support wrangler.toml", async () => { const helper = new WranglerE2ETestHelper(); await helper.seed({ @@ -315,61 +250,6 @@ describe("pages dev", () => { ).resolves.toMatchInlineSnapshot('"Updated Worker!"'); }); - it("should support modifying _routes.json during dev session", async () => { - const helper = new WranglerE2ETestHelper(); - await helper.seed({ - "_worker.js": dedent` - export default { - async fetch(request, env) { - const url = new URL(request.url); - if (url.pathname === "/foo") { - return new Response("foo"); - } - if (url.pathname === "/bar") { - return new Response("bar"); - } - return new Response("Hello _routes.json") - } - }`, - "_routes.json": dedent` - { - "version": 1, - "include": ["/foo", "/bar"], - "exclude": [] - } - `, - "index.html": dedent` - hello world - `, - }); - const port = await getPort(); - const worker = helper.runLongLived(`wrangler pages dev --port ${port} .`); - const { url } = await worker.waitForReady(); - - const foo = await fetchText(`${url}/foo`); - expect(foo).toMatchInlineSnapshot('"foo"'); - - const bar = await fetchText(`${url}/bar`); - expect(bar).toMatchInlineSnapshot('"bar"'); - - await helper.seed({ - "_routes.json": dedent` - { - "version": 1, - "include": ["/foo"], - "exclude": ["/bar"] - } - `, - }); - await worker.waitForReload(); - - const foo2 = await fetchText(`${url}/foo`); - expect(foo2).toMatchInlineSnapshot('"foo"'); - - const bar2 = await fetchText(`${url}/bar`); - expect(bar2).toMatchInlineSnapshot('"hello world"'); - }); - it("should validate _routes.json during dev session, and fallback to default value", async () => { const helper = new WranglerE2ETestHelper(); await helper.seed({ @@ -593,6 +473,7 @@ describe("pages dev", () => { it("should pick up wrangler.toml configuration even in cases when `pages_build_output_dir` was not specified, but the command argument was", async () => { const helper = new WranglerE2ETestHelper(); + await helper.seed({ "public/_worker.js": dedent` export default { @@ -608,13 +489,216 @@ describe("pages dev", () => { PAGES_EMOJI = "⚡️" `, }); + const port = await getPort(); const worker = helper.runLongLived( `wrangler pages dev --port ${port} public` ); const { url } = await worker.waitForReady(); + await expect(fetchText(url)).resolves.toMatchInlineSnapshot( `"⚡️ Pages supports wrangler.toml ⚡️"` ); }); + + describe("watch mode", () => { + it("should modify worker during dev session (Functions)", async () => { + const helper = new WranglerE2ETestHelper(); + + await helper.seed({ + "functions/_middleware.js": dedent` + export async function onRequest() { + return new Response("Hello World!") + }`, + }); + + const port = await getPort(); + const worker = helper.runLongLived(`wrangler pages dev --port ${port} .`); + const { url } = await worker.waitForReady(); + + let text = await fetchText(url); + expect(text).toMatchInlineSnapshot('"Hello World!"'); + + await helper.seed({ + "functions/_middleware.js": dedent` + export async function onRequest() { + return new Response("Updated Worker!") + }`, + }); + + await worker.waitForReload(); + + text = await fetchText(url); + expect(text).toMatchInlineSnapshot('"Updated Worker!"'); + }); + + it("should support modifying dependencies during dev session (Functions)", async () => { + const helper = new WranglerE2ETestHelper(); + + await helper.seed({ + "utils/greetings.js": dedent` + export const hello = "Hello World!" + export const hi = "Hi there!" + `, + "functions/greetings/_middleware.js": dedent` + import { hello } from "../../utils/greetings" + export async function onRequest() { + return new Response(hello) + }`, + "functions/hi.js": dedent` + import { hi } from "../utils/greetings" + export async function onRequest() { + return new Response(hi) + }`, + }); + + const port = await getPort(); + const worker = helper.runLongLived(`wrangler pages dev --port ${port} .`); + const { url } = await worker.waitForReady(); + + let hello = await fetchText(`${url}/greetings/hello`); + expect(hello).toMatchInlineSnapshot('"Hello World!"'); + + let hi = await fetchText(`${url}/hi`); + expect(hi).toMatchInlineSnapshot('"Hi there!"'); + + await helper.seed({ + "utils/greetings.js": dedent` + export const hello = "Hey World!" + export const hi = "Hey there!" + `, + }); + + await worker.waitForReload(); + + hello = await fetchText(`${url}/greetings/hello`); + expect(hello).toMatchInlineSnapshot('"Hey World!"'); + + hi = await fetchText(`${url}/hi`); + expect(hi).toMatchInlineSnapshot('"Hey there!"'); + }); + + it("should support modifying external modules during dev session (Functions)", async () => { + const helper = new WranglerE2ETestHelper(); + + await helper.seed({ + "modules/my-html.html": dedent` +

Hello HTML World!

+ `, + "functions/hello.js": dedent` + import html from "../modules/my-html.html"; + export async function onRequest() { + return new Response(html); + }`, + }); + + const port = await getPort(); + const worker = helper.runLongLived(`wrangler pages dev --port ${port} .`); + const { url } = await worker.waitForReady(); + + let hello = await fetchText(`${url}/hello`); + expect(hello).toMatchInlineSnapshot('"

Hello HTML World!

"'); + + await helper.seed({ + "modules/my-html.html": dedent` +

Updated HTML!

+ `, + }); + + await worker.waitForReload(); + + hello = await fetchText(`${url}/hello`); + expect(hello).toMatchInlineSnapshot('"

Updated HTML!

"'); + }); + + it("should modify worker during dev session (_worker)", async () => { + const helper = new WranglerE2ETestHelper(); + + await helper.seed({ + "_worker.js": dedent` + export default { + fetch(request, env) { + return new Response("Hello World!") + } + }`, + }); + + const port = await getPort(); + const worker = helper.runLongLived(`wrangler pages dev --port ${port} .`); + const { url } = await worker.waitForReady(); + + let hello = await fetchText(url); + expect(hello).toMatchInlineSnapshot('"Hello World!"'); + + await helper.seed({ + "_worker.js": dedent` + export default { + fetch(request, env) { + return new Response("Updated Worker!") + } + }`, + }); + + await worker.waitForReload(); + + hello = await fetchText(url); + expect(hello).toMatchInlineSnapshot('"Updated Worker!"'); + }); + + it("should support modifying _routes.json during dev session", async () => { + const helper = new WranglerE2ETestHelper(); + + await helper.seed({ + "_worker.js": dedent` + export default { + async fetch(request, env) { + const url = new URL(request.url); + if (url.pathname === "/foo") { + return new Response("foo"); + } + if (url.pathname === "/bar") { + return new Response("bar"); + } + return new Response("Hello _routes.json") + } + }`, + "_routes.json": dedent` + { + "version": 1, + "include": ["/foo", "/bar"], + "exclude": [] + } + `, + "index.html": dedent` + hello world + `, + }); + const port = await getPort(); + const worker = helper.runLongLived(`wrangler pages dev --port ${port} .`); + const { url } = await worker.waitForReady(); + + const foo = await fetchText(`${url}/foo`); + expect(foo).toMatchInlineSnapshot('"foo"'); + + const bar = await fetchText(`${url}/bar`); + expect(bar).toMatchInlineSnapshot('"bar"'); + + await helper.seed({ + "_routes.json": dedent` + { + "version": 1, + "include": ["/foo"], + "exclude": ["/bar"] + } + `, + }); + await worker.waitForReload(); + + const foo2 = await fetchText(`${url}/foo`); + expect(foo2).toMatchInlineSnapshot('"foo"'); + + const bar2 = await fetchText(`${url}/bar`); + expect(bar2).toMatchInlineSnapshot('"hello world"'); + }); + }); }); diff --git a/packages/wrangler/src/pages/dev.ts b/packages/wrangler/src/pages/dev.ts index 05816e9218fa..15bb9595c535 100644 --- a/packages/wrangler/src/pages/dev.ts +++ b/packages/wrangler/src/pages/dev.ts @@ -1,6 +1,6 @@ import { execSync, spawn } from "node:child_process"; import { existsSync, lstatSync, readFileSync } from "node:fs"; -import { join, resolve } from "node:path"; +import { join, normalize, resolve } from "node:path"; import { watch } from "chokidar"; import * as esbuild from "esbuild"; import { unstable_dev } from "../api"; @@ -29,7 +29,7 @@ import { produceWorkerBundleForWorkerJSDirectory, } from "./functions/buildWorker"; import { validateRoutes } from "./functions/routes-validation"; -import { CLEANUP, CLEANUP_CALLBACKS, getPagesTmpDir } from "./utils"; +import { CLEANUP, CLEANUP_CALLBACKS, debounce, getPagesTmpDir } from "./utils"; import type { Config } from "../config"; import type { DurableObjectBindings, @@ -462,9 +462,54 @@ export const Handler = async (args: PagesDevArguments) => { logger.debug(`Compiling worker to "${scriptPath}"...`); const onEnd = () => scriptReadyResolve(); + const watchedBundleDependencies: Set = new Set(); + + /* + * 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 needed. + */ + // always watch the "/functions" directory + const watcher = watch([functionsDirectory], { + persistent: true, + ignoreInitial: true, + }); + try { const buildFn = async () => { - await buildFunctions({ + const currentBundleDependencies = new Set(); + + const bundle = await buildFunctions({ outfile: scriptPath, functionsDirectory, sourcemap: true, @@ -476,43 +521,134 @@ export const Handler = async (args: PagesDevArguments) => { routesModule, defineNavigatorUserAgent, }); + + for (const dep in bundle.dependencies) { + const resolvedDep = resolve(functionsDirectory, dep); + /* + * EXCLUDE: + * - the "/functions" directory because we're already watching it + * - everything in "./.wrangler", as it's mostly cache and + * temporary files + * - any bundle dependencies we are already watching + * - anything outside of the current working directory, since we + * are expecting `wrangler pages dev` to be run from the Pages + * project root folder + */ + if ( + !resolvedDep.includes(normalize("/functions/")) && + !resolvedDep.includes(normalize("/.wrangler/")) && + resolvedDep.includes(resolve(process.cwd())) + ) { + currentBundleDependencies.add(resolvedDep); + + if (!watchedBundleDependencies.has(resolvedDep)) { + watchedBundleDependencies.add(resolvedDep); + watcher.add(resolvedDep); + } + } + } + + // handle non-JS module dependencies, such as wasm/html/binary imports + for (const module of bundle.modules) { + if (module.filePath) { + const resolvedDep = resolve(functionsDirectory, module.filePath); + currentBundleDependencies.add(resolvedDep); + + if (!watchedBundleDependencies.has(resolvedDep)) { + watchedBundleDependencies.add(resolvedDep); + watcher.add(resolvedDep); + } + } + } + + /* + *`bundle.dependencies` and `bundle.modules` will always contain the + * latest dependency list of the current bundle. If we are currently + * watching any dependency files not in that list, we should remove + * them, as they are no longer relevant to the compiled Functions. + */ + watchedBundleDependencies.forEach(async (path) => { + if (!currentBundleDependencies.has(path)) { + watchedBundleDependencies.delete(path); + watcher.unwatch(path); + } + }); + await metrics.sendMetricsEvent("build pages functions"); }; - await buildFn(); - // If Functions found routes, continue using Functions - watch([functionsDirectory], { - persistent: true, - ignoreInitial: true, - }).on("all", async () => { + /* + * Improve developer experience by debouncing the re-building + * of Functions. + * + * "Debouncing ensures that exactly one signal is sent for an + * event that may be happening several times — or even several + * hundreds of times over an extended period. As long as the + * events are occurring fast enough to happen at least once in + * every detection period, the signal will not be sent!" + * (http://unscriptable.com/2009/03/20/debouncing-javascript-methods/) + * + * This handles use cases such as bulk file/directory changes + * (such as copy/pasting multiple files/directories), where + * chokidar will trigger a change event per each changed file/ + * directory. In such use cases, we want to ensure that we + * re-build Functions once, as opposed to once per change event. + */ + const debouncedBuildFn = debounce(async () => { try { await buildFn(); } catch (e) { if (e instanceof FunctionsNoRoutesError) { - logger.warn( + logger.error( getFunctionsNoRoutesWarning(functionsDirectory, "skipping") ); } else if (e instanceof FunctionsBuildError) { - logger.warn( + logger.error( getFunctionsBuildWarning(functionsDirectory, e.message) ); } else { - throw e; + /* + * don't break developer flow in watch mode by throwing an error + * here. Many times errors will be just the result of unfinished + * typing. Instead, log the error, point out we are still serving + * the last successfully built Functions, and allow developers to + * write their code to completion + */ + logger.error( + `Error while attempting to build the Functions directory ${functionsDirectory}. Skipping to last successfully built version of Functions.\n` + + `${e}` + ); } } + }, 50); + + await buildFn(); + + // If Functions found routes, continue using Functions + watcher.on("all", async (eventName, path) => { + logger.debug(`🌀 "${eventName}" event detected at ${path}.`); + + debouncedBuildFn(); }); } catch (e) { // If there are no Functions, then Pages will only serve assets. if (e instanceof FunctionsNoRoutesError) { - logger.warn( + logger.error( getFunctionsNoRoutesWarning(functionsDirectory, "skipping") ); // Resolve anyway and run without Functions onEnd(); - // Turn off Functions usingFunctions = false; } else { - throw e; + /* + * do not start the `pages dev` session if we encounter errors + * while attempting to build Functions. These flag underlying + * issues in the Functions code, and should be addressed before + */ + throw new FatalError( + `Error while attempting to build the Functions directory ${functionsDirectory}\n` + + `${e}` + ); } } } diff --git a/packages/wrangler/src/pages/functions/buildWorker.ts b/packages/wrangler/src/pages/functions/buildWorker.ts index 34aa335964e0..6427b5a99d53 100644 --- a/packages/wrangler/src/pages/functions/buildWorker.ts +++ b/packages/wrangler/src/pages/functions/buildWorker.ts @@ -9,7 +9,7 @@ import { noopModuleCollector, } from "../../deployment-bundle/module-collection"; import { FatalError } from "../../errors"; -import { logger } from "../../logger"; +import { logBuildFailure, logger } from "../../logger"; import { getBasePath } from "../../paths"; import { getPagesProjectRoot, getPagesTmpDir } from "../utils"; import type { BundleResult } from "../../deployment-bundle/bundle"; @@ -351,19 +351,13 @@ export function buildNotifierPlugin(onEnd: () => void): Plugin { name: "wrangler notifier and monitor", setup(pluginBuild) { pluginBuild.onEnd((result) => { - if (result.errors.length > 0) { - logger.error( - `${result.errors.length} error(s) and ${result.warnings.length} warning(s) when compiling Worker.` - ); - } else if (result.warnings.length > 0) { - logger.warn( - `${result.warnings.length} warning(s) when compiling Worker.` - ); - onEnd(); + if (result.errors.length > 0 || result.warnings.length > 0) { + logBuildFailure(result.errors, result.warnings); } else { logger.log("✨ Compiled Worker successfully"); - onEnd(); } + + onEnd(); }); }, }; diff --git a/packages/wrangler/src/pages/utils.ts b/packages/wrangler/src/pages/utils.ts index 95a9a177dbaa..df227ade1eb0 100644 --- a/packages/wrangler/src/pages/utils.ts +++ b/packages/wrangler/src/pages/utils.ts @@ -75,3 +75,22 @@ export function getPagesTmpDir(): string { tmpDirCacheProjectRoot = projectRoot; return tmpDirCache; } + +/** + * Creates a basic debounced function that delays invoking `fn` until after + * `delayMs` milliseconds have elapsed since the last time the debounced + * function was invoked. + */ +export function debounce(fn: () => void, delayMs = 100) { + let crrTimeoutId: NodeJS.Timeout | undefined; + + return () => { + if (crrTimeoutId) { + clearTimeout(crrTimeoutId); + } + + crrTimeoutId = setTimeout(() => { + fn(); + }, delayMs); + }; +}