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

[wrangler] fix: stop rebuild attempts when sources are missing (#3886) #4877

Merged
10 changes: 10 additions & 0 deletions .changeset/hip-files-count.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
"wrangler": minor
---

fix: Do not show unnecessary errors during watch rebuilds

When Pages is used in conjunction with a full stack framework, the framework
build will temporarily remove files that are being watched by Pages, such as
`_worker.js` and `_routes.json`.
Previously we would display errors for these changes, which adds confusing and excessive messages to the Pages dev output. Now builds are skipped if a watched `_worker.js` or `_routes.json` is removed.
93 changes: 93 additions & 0 deletions fixtures/pages-workerjs-app/tests/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { execSync } from "node:child_process";
import { rename } from "node:fs/promises";
import path, { resolve } from "node:path";
import { setTimeout } from "node:timers/promises";
import { fetch } from "undici";
import { describe, it } from "vitest";
import { runWranglerPagesDev } from "../../shared/src/run-wrangler-long-lived";
Expand Down Expand Up @@ -60,4 +62,95 @@ describe("Pages _worker.js", () => {
await stop();
}
});

it("should not error if the worker.js file is removed while watching", async ({
expect,
}) => {
const basePath = resolve(__dirname, "..");
const { ip, port, getOutput, clearOutput, stop } =
await runWranglerPagesDev(resolve(__dirname, ".."), "./workerjs-test", [
"--port=0",
"--inspector-port=0",
]);
try {
clearOutput();
await tryRename(
basePath,
"workerjs-test/_worker.js",
"workerjs-test/XXX_worker.js"
);
await setTimeout(1000);
// Expect no output since the deletion of the worker should be ignored
expect(getOutput()).toBe("");
await tryRename(
basePath,
"workerjs-test/XXX_worker.js",
"workerjs-test/_worker.js"
);
await setTimeout(1000);
// Expect replacing the worker to now trigger a success build.
expect(getOutput()).toContain("Compiled Worker successfully");
} finally {
await stop();
await tryRename(
basePath,
"workerjs-test/XXX_worker.js",
"workerjs-test/_worker.js"
);
}
});

it("should not error if the _routes.json file is removed while watching", async ({
expect,
}) => {
const basePath = resolve(__dirname, "..");
const { ip, port, getOutput, clearOutput, stop } =
await runWranglerPagesDev(resolve(__dirname, ".."), "./workerjs-test", [
"--port=0",
"--inspector-port=0",
]);
try {
clearOutput();
await tryRename(
basePath,
"workerjs-test/_routes.json",
"workerjs-test/XXX_routes.json"
);
await setTimeout(1000);
// Expect no output since the deletion of the routes file should be ignored
expect(getOutput()).toBe("");
await tryRename(
basePath,
"workerjs-test/XXX_routes.json",
"workerjs-test/_routes.json"
);
await setTimeout(1000);
// Expect replacing the routes file to trigger a build, although
// the routes build does not provide any output feedback to compare against,
// so we just check that nothing else is being printed.
expect(getOutput()).toBe("");
} finally {
await stop();
await tryRename(
basePath,
"workerjs-test/XXX_routes.json",
"workerjs-test/_routes.json"
);
}
});

async function tryRename(
basePath: string,
from: string,
to: string
): Promise<void> {
try {
await rename(resolve(basePath, from), resolve(basePath, to));
} catch (e) {
// Do nothing if the file was not found
if ((e as any).code !== "ENOENT") {
throw e;
}
}
}
});
6 changes: 6 additions & 0 deletions fixtures/pages-workerjs-app/workerjs-test/_routes.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"version": 1,
"description": "",
"include": ["/*"],
"exclude": []
}
3 changes: 2 additions & 1 deletion fixtures/shared/src/run-wrangler-long-lived.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ async function runLongLivedWrangler(command: string[], cwd: string) {
chunks.push(chunk);
});
const getOutput = () => Buffer.concat(chunks).toString();
const clearOutput = () => (chunks.length = 0);

const timeoutHandle = setTimeout(() => {
if (settledReadyPromise) return;
Expand Down Expand Up @@ -90,5 +91,5 @@ async function runLongLivedWrangler(command: string[], cwd: string) {
}

const { ip, port } = await ready;
return { ip, port, stop, getOutput };
return { ip, port, stop, getOutput, clearOutput };
}
10 changes: 8 additions & 2 deletions packages/wrangler/src/pages/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,10 @@
watch([workerScriptPath], {
persistent: true,
ignoreInitial: true,
}).on("all", async () => {
}).on("all", async (event) => {

Check warning on line 374 in packages/wrangler/src/pages/dev.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/pages/dev.ts#L374

Added line #L374 was not covered by tests
if (event === "unlink") {
return;

Check warning on line 376 in packages/wrangler/src/pages/dev.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/pages/dev.ts#L376

Added line #L376 was not covered by tests
}
await runBuild();
});
} else if (usingFunctions) {
Expand Down Expand Up @@ -539,8 +542,11 @@
watch([routesJSONPath], {
persistent: true,
ignoreInitial: true,
}).on("all", async () => {
}).on("all", async (event) => {

Check warning on line 545 in packages/wrangler/src/pages/dev.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/pages/dev.ts#L545

Added line #L545 was not covered by tests
try {
if (event === "unlink") {
return;

Check warning on line 548 in packages/wrangler/src/pages/dev.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/pages/dev.ts#L548

Added line #L548 was not covered by tests
}
/**
* Watch for _routes.json file changes and validate file each time.
* If file is valid proceed to running the build.
Expand Down
Loading