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 assets + custom build = infinite loop #6881

Merged
merged 5 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/happy-balloons-work.md
Original file line number Diff line number Diff line change
@@ -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
60 changes: 60 additions & 0 deletions packages/wrangler/e2e/dev.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
21 changes: 21 additions & 0 deletions packages/wrangler/src/api/startDevWorker/BundlerController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -259,6 +260,24 @@ export class BundlerController extends Controller<BundlerControllerEventMap> {
);
}

#assetsWatcher?: ReturnType<typeof watch>;
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) {
Expand All @@ -280,6 +299,7 @@ export class BundlerController extends Controller<BundlerControllerEventMap> {

void this.#startCustomBuild(event.config);
void this.#startBundle(event.config);
void this.#ensureWatchingAssets(event.config);
}

async teardown() {
Expand All @@ -289,6 +309,7 @@ export class BundlerController extends Controller<BundlerControllerEventMap> {
await Promise.all([
this.#bundlerCleanup?.(),
this.#customBuildWatcher?.close(),
this.#assetsWatcher?.close(),
]);
logger.debug("BundlerController teardown complete");
}
Expand Down
30 changes: 1 addition & 29 deletions packages/wrangler/src/api/startDevWorker/ConfigController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -358,7 +357,6 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
latestConfig?: StartDevWorkerOptions;

#configWatcher?: ReturnType<typeof watch>;
#assetsWatcher?: ReturnType<typeof watch>;
#abortController?: AbortController;

async #ensureWatchingConfig(configPath: string | undefined) {
Expand All @@ -378,24 +376,6 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
}
}

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);
}
Expand Down Expand Up @@ -442,11 +422,6 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {
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;
Expand Down Expand Up @@ -476,10 +451,7 @@ export class ConfigController extends Controller<ConfigControllerEventMap> {

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");
}

Expand Down
Loading