From 7ca37bcbb274e88709fc14aea6f62c003ddc1b92 Mon Sep 17 00:00:00 2001 From: Rahul Sethi <5822355+RamIdeas@users.noreply.github.com> Date: Thu, 3 Oct 2024 17:13:14 +0100 Subject: [PATCH] fix assets + custom build = infinite loop (#6881) --- .changeset/happy-balloons-work.md | 5 ++ packages/wrangler/e2e/dev.test.ts | 60 +++++++++++++++++++ .../api/startDevWorker/BundlerController.ts | 21 +++++++ .../api/startDevWorker/ConfigController.ts | 30 +--------- 4 files changed, 87 insertions(+), 29 deletions(-) create mode 100644 .changeset/happy-balloons-work.md diff --git a/.changeset/happy-balloons-work.md b/.changeset/happy-balloons-work.md new file mode 100644 index 000000000000..4bc186bf9763 --- /dev/null +++ b/.changeset/happy-balloons-work.md @@ -0,0 +1,5 @@ +--- +"wrangler": patch +--- + +fix: custom builds outputting files in assets watched directory no longer cause the custom build to run again in an infinite loop diff --git a/packages/wrangler/e2e/dev.test.ts b/packages/wrangler/e2e/dev.test.ts index d086b032ed5c..1a58ec632fd0 100644 --- a/packages/wrangler/e2e/dev.test.ts +++ b/packages/wrangler/e2e/dev.test.ts @@ -877,6 +877,66 @@ describe("custom builds", () => { text = await fetchText(url); expect(text).toMatchInlineSnapshot(`"Hello, World!"`); }); + + it("does not infinite-loop custom build with assets", async () => { + const helper = new WranglerE2ETestHelper(); + await helper.seed({ + "wrangler.toml": dedent` + name = "${workerName}" + compatibility_date = "2023-01-01" + main = "src/index.ts" + build.command = "echo 'hello' > ./public/index.html" + + [assets] + directory = "./public" + `, + "src/index.ts": dedent` + export default { + async fetch(request) { + return new Response("Hello, World!") + } + } + `, + "public/other.html": "ensure ./public exists", + }); + const worker = helper.runLongLived("wrangler dev"); + + // first build on startup + await worker.readUntil(/Running custom build/, 5_000); + // second build for first watcher notification (can be optimised away, leaving as-is for now) + await worker.readUntil(/Running custom build/, 5_000); + + // Need to get the url in this order because waitForReady calls readUntil + // which keeps track of where it's read up to so far, + // so the expect(waitUntil).reject assertion below + // will eat up the "Ready on http://localhost:8787" message if called before. + // This could cause a flake if eg the 2nd custom build starts after ready. + const { url } = await worker.waitForReady(); + + // assert no more custom builds happen + // regression: https://github.com/cloudflare/workers-sdk/issues/6876 + await expect( + worker.readUntil(/Running custom build:/, 5_000) + ).rejects.toThrowError(); + + // now check assets are still fetchable, even after updates + + const res = await fetch(url); + await expect(res.text()).resolves.toBe("hello\n"); + + await helper.seed({ + "public/index.html": "world", + }); + + const resText = await retry( + (text) => text === "hello\n", + async () => { + const res2 = await fetch(url); + return res2.text(); + } + ); + await expect(resText).toBe("world"); + }); }); describe("watch mode", () => { diff --git a/packages/wrangler/src/api/startDevWorker/BundlerController.ts b/packages/wrangler/src/api/startDevWorker/BundlerController.ts index dfcdb738116a..c327ac9ebfbc 100644 --- a/packages/wrangler/src/api/startDevWorker/BundlerController.ts +++ b/packages/wrangler/src/api/startDevWorker/BundlerController.ts @@ -10,6 +10,7 @@ import { getWrangler1xLegacyModuleReferences, } from "../../deployment-bundle/module-collection"; import { runCustomBuild } from "../../deployment-bundle/run-custom-build"; +import { getAssetChangeMessage } from "../../dev"; import { runBuild } from "../../dev/use-esbuild"; import { logger } from "../../logger"; import { isNavigatorDefined } from "../../navigator-user-agent"; @@ -259,6 +260,24 @@ export class BundlerController extends Controller { ); } + #assetsWatcher?: ReturnType; + async #ensureWatchingAssets(config: StartDevWorkerOptions) { + await this.#assetsWatcher?.close(); + + if (config.assets?.directory) { + this.#assetsWatcher = watch(config.assets.directory, { + persistent: true, + ignoreInitial: true, + }).on("all", async (eventName, filePath) => { + const message = getAssetChangeMessage(eventName, filePath); + logger.debug(`🌀 ${message}...`); + if (this.#currentBundle) { + this.emitBundleCompleteEvent(config, this.#currentBundle); + } + }); + } + } + #tmpDir?: EphemeralDirectory; onConfigUpdate(event: ConfigUpdateEvent) { @@ -280,6 +299,7 @@ export class BundlerController extends Controller { void this.#startCustomBuild(event.config); void this.#startBundle(event.config); + void this.#ensureWatchingAssets(event.config); } async teardown() { @@ -289,6 +309,7 @@ export class BundlerController extends Controller { await Promise.all([ this.#bundlerCleanup?.(), this.#customBuildWatcher?.close(), + this.#assetsWatcher?.close(), ]); logger.debug("BundlerController teardown complete"); } diff --git a/packages/wrangler/src/api/startDevWorker/ConfigController.ts b/packages/wrangler/src/api/startDevWorker/ConfigController.ts index 777f13809107..49ea954ee5b9 100644 --- a/packages/wrangler/src/api/startDevWorker/ConfigController.ts +++ b/packages/wrangler/src/api/startDevWorker/ConfigController.ts @@ -13,7 +13,6 @@ import { processAssetsArg, validateAssetsArgsAndConfig } from "../../assets"; import { printBindings, readConfig } from "../../config"; import { getEntry } from "../../deployment-bundle/entry"; import { - getAssetChangeMessage, getBindings, getHostAndRoutes, getInferredHost, @@ -358,7 +357,6 @@ export class ConfigController extends Controller { latestConfig?: StartDevWorkerOptions; #configWatcher?: ReturnType; - #assetsWatcher?: ReturnType; #abortController?: AbortController; async #ensureWatchingConfig(configPath: string | undefined) { @@ -378,24 +376,6 @@ export class ConfigController extends Controller { } } - async #ensureWatchingAssets(assetsPath: string | undefined) { - await this.#assetsWatcher?.close(); - - if (assetsPath) { - this.#assetsWatcher = watch(assetsPath, { - persistent: true, - ignoreInitial: true, - }).on("all", async (eventName, filePath) => { - const message = getAssetChangeMessage(eventName, filePath); - logger.debug(`🌀 ${message}...`); - assert( - this.latestInput, - "Cannot be watching config without having first set an input" - ); - void this.#updateConfig(this.latestInput); - }); - } - } public set(input: StartDevWorkerInput, throwErrors = false) { return this.#updateConfig(input, throwErrors); } @@ -442,11 +422,6 @@ export class ConfigController extends Controller { void this.#ensureWatchingConfig(fileConfig.configPath); } - const assets = processAssetsArg({ assets: input?.assets }, fileConfig); - if (assets && typeof vitest === "undefined") { - void this.#ensureWatchingAssets(assets.directory); - } - const resolvedConfig = await resolveConfig(fileConfig, input); if (signal.aborted) { return; @@ -476,10 +451,7 @@ export class ConfigController extends Controller { async teardown() { logger.debug("ConfigController teardown beginning..."); - await Promise.allSettled([ - this.#configWatcher?.close(), - this.#assetsWatcher?.close(), - ]); + await this.#configWatcher?.close(); logger.debug("ConfigController teardown complete"); }