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(pages): wrangler pages dev should prioritize _worker.js #1950

Merged
merged 1 commit into from
Oct 4, 2022
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
9 changes: 9 additions & 0 deletions .changeset/silent-swans-relax.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"pages-workerjs-and-functions-app": patch
"wrangler": patch
---

`wrangler pages dev` should prioritize `_worker.js`

When using a `_worker.js` file, the entire `/functions` directory should be ignored – this includes its routing and middleware characteristics. Currently `wrangler pages dev` does the reverse, by prioritizing
`/functions` over `_worker.js`. These changes fix the current behaviour.
32 changes: 32 additions & 0 deletions fixtures/pages-workerjs-and-functions-app/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# ⚡️ pages-workerjs-and-functions-app

`pages-workerjs-and-functions-app` is a test fixture that sets up an [Advanced Mode](https://developers.cloudflare.com/pages/platform/functions/#advanced-mode) ⚡️Pages project that also includes a `/functions` directory. This fixture is meant to test that for such projects, the single worker script provided by users will
always take precedence over the `functions` directory.

## Publish

> Please note that in order to deploy this project to `.pages.dev` you need to have a [Cloudflare account](https://dash.cloudflare.com/login)

```bash
# cd into the test fixture folder
cd fixtures/pages-workerjs-and-functions-app

# Deploy the directory of static assets as a Pages deployment
npx wrangler pages publish public
```

If deployment was successful, follow the URL refrenced in the success message in your terminal

```
✨ Deployment complete! Take a peek over at https:/<hash>.<PROJECT_NAME>.pages.dev
```

## Run tests

```bash
# cd into the test fixture folder
cd fixtures/pages-workerjs-and-functions-app

# Run tests
npm run test
```
3 changes: 3 additions & 0 deletions fixtures/pages-workerjs-and-functions-app/functions/date.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export async function onRequest() {
return new Response(new Date().toISOString());
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export async function onRequest() {
return new Response("Hello world!");
}
33 changes: 33 additions & 0 deletions fixtures/pages-workerjs-and-functions-app/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"name": "pages-workerjs-and-functions-app",
"version": "0.0.0",
"private": true,
"sideEffects": false,
"scripts": {
"dev": "npx wrangler pages dev public --port 8955",
"test": "npx jest --forceExit --verbose",
"test:ci": "npx jest --forceExit"
},
"jest": {
"restoreMocks": true,
"testRegex": ".*.(test|spec)\\.[jt]sx?$",
"testTimeout": 30000,
"transform": {
"^.+\\.c?(t|j)sx?$": [
"esbuild-jest",
{
"sourcemap": true
}
]
},
"transformIgnorePatterns": [
"node_modules/(?!find-up|locate-path|p-locate|p-limit|yocto-queue|path-exists|execa|strip-final-newline|npm-run-path|path-key|onetime|mimic-fn|human-signals|is-stream)"
]
},
"devDependencies": {
"undici": "^5.9.1"
},
"engines": {
"node": ">=16.13"
}
}
24 changes: 24 additions & 0 deletions fixtures/pages-workerjs-and-functions-app/public/_worker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
export default {
async fetch(request, env) {
const url = new URL(request.url);
if (url.pathname.startsWith("/greeting/hello")) {
return new Response("Bonjour le monde!");
}

if (url.pathname.startsWith("/greeting/goodbye")) {
return new Response("A plus tard alligator 👋");
}

if (url.pathname.startsWith("/party")) {
return new Response("Oops! Tous les alligators sont allés à la fête 🎉");
}

if (url.pathname.startsWith("/date")) {
return new Response(
"Yesterday is history, tomorrow is a mystery, but today is a gift. That’s why it is called the present."
Copy link
Contributor

Choose a reason for hiding this comment

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

🙇

);
}

return env.ASSETS.fetch(request);
},
};
8 changes: 8 additions & 0 deletions fixtures/pages-workerjs-and-functions-app/public/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<!DOCTYPE html>
<html>
<body>
<h1>
Bienvenue sur notre projet &#10024; pages-workerjs-and-functions-app!
</h1>
</body>
</html>
84 changes: 84 additions & 0 deletions fixtures/pages-workerjs-and-functions-app/tests/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { spawn } from "child_process";
import * as path from "path";
import patchConsole from "patch-console";
import { fetch } from "undici";
import type { ChildProcess } from "child_process";
import type { Response } from "undici";

const waitUntilReady = async (url: string): Promise<Response> => {
let response: Response | undefined = undefined;

while (response === undefined) {
await new Promise((resolvePromise) => setTimeout(resolvePromise, 500));

try {
response = await fetch(url);
} catch (err) {}
}

return response as Response;
};

const isWindows = process.platform === "win32";

describe("Pages project with `_worker.js` and `/functions` directory", () => {
let wranglerProcess: ChildProcess;

// const std = mockConsoleMethods();
beforeEach(() => {
wranglerProcess = spawn("npm", ["run", "dev"], {
shell: isWindows,
cwd: path.resolve(__dirname, "../"),
env: { BROWSER: "none", ...process.env },
});
wranglerProcess.stdout?.on("data", (chunk) => {
console.log(chunk.toString());
});
wranglerProcess.stderr?.on("data", (chunk) => {
console.log(chunk.toString());
});
});

afterEach(async () => {
patchConsole(() => {});

await new Promise((resolve, reject) => {
wranglerProcess.once("exit", (code) => {
if (!code) {
resolve(code);
} else {
reject(code);
}
});
wranglerProcess.kill("SIGTERM");
});
});

it("renders static pages", async () => {
const response = await waitUntilReady("http://127.0.0.1:8955/");
const text = await response.text();
expect(text).toContain(
"Bienvenue sur notre projet &#10024; pages-workerjs-and-functions-app!"
);
});

it("runs our _worker.js and ignores the functions directory", async () => {
let response = await waitUntilReady("http://127.0.0.1:8955/greeting/hello");
let text = await response.text();
expect(text).toEqual("Bonjour le monde!");

response = await waitUntilReady("http://127.0.0.1:8955/greeting/goodbye");
text = await response.text();
expect(text).toEqual("A plus tard alligator 👋");

response = await waitUntilReady("http://127.0.0.1:8955/date");
text = await response.text();
expect(text).toEqual(
"Yesterday is history, tomorrow is a mystery, but today is a gift. That’s why it is called the present."
);

response = await waitUntilReady("http://127.0.0.1:8955/party");
text = await response.text();
expect(text).toEqual("Oops! Tous les alligators sont allés à la fête 🎉");
});
});
10 changes: 10 additions & 0 deletions fixtures/pages-workerjs-and-functions-app/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"compilerOptions": {
"target": "ES2020",
"esModuleInterop": true,
"module": "CommonJS",
"lib": ["ES2020"],
"types": ["jest"],
"moduleResolution": "node"
}
}
71 changes: 37 additions & 34 deletions packages/wrangler/src/pages/dev.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,6 @@ export const Handler = async ({
throw new FatalError("Pages does not support wrangler.toml", 1);
}

const functionsDirectory = "./functions";
let usingFunctions = existsSync(functionsDirectory);
let usingWorkerScript = false;

const command = remaining;

let proxyPort: number | undefined;
Expand Down Expand Up @@ -236,10 +232,42 @@ export const Handler = async ({
(promiseResolve) => (scriptReadyResolve = promiseResolve)
);

const workerScriptPath =
directory !== undefined
? join(directory, singleWorkerScriptPath)
: singleWorkerScriptPath;
const usingWorkerScript = existsSync(workerScriptPath);

const functionsDirectory = "./functions";
let usingFunctions = !usingWorkerScript && existsSync(functionsDirectory);

let scriptPath = "";

// Try to use Functions
if (usingFunctions) {
if (usingWorkerScript) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
scriptReadyResolve!();

scriptPath = workerScriptPath;

const runBuild = async () => {
try {
await esbuild.build({
entryPoints: [scriptPath],
write: false,
plugins: [blockWorkerJsImports],
});
} catch {}
};

await runBuild();
watch([scriptPath], {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this watch only get cancelled if you kill the process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petebacondarwin that is correct. In fact that is true for all our watchers. I am rather new to watch and esbuild so my reasoning might be wrong, but the way I see it is that we want to keep watching these files (_worker.js, /functions/* and _routes.json) for changes for as long as the process is running, so we can serve the right assets.
Is there a use case you had in mind that would require us to close the watcher? Or what was the underlying concern of your question?

Copy link
Contributor

Choose a reason for hiding this comment

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

My only thought was whether esbuild kicks off a child process that might somehow get orphaned if we closed down the process in some other way than killing it? But I think that is probably not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to evanw/esbuild#643 this 👉 https://github.com/evanw/esbuild/blob/master/cmd/esbuild/service.go#L124-L137 should prevent something like that from happening. So I think we should be OK

persistent: true,
ignoreInitial: true,
}).on("all", async () => {
await runBuild();
});
} else if (usingFunctions) {
// Try to use Functions
const outfile = join(tmpdir(), `./functionsWorker-${Math.random()}.js`);
scriptPath = outfile;

Expand Down Expand Up @@ -307,37 +335,12 @@ export const Handler = async ({

// Depending on the result of building Functions, we may not actually be using
// Functions even if the directory exists.
if (!usingFunctions) {
if (!usingFunctions && !usingWorkerScript) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
scriptReadyResolve!();

scriptPath =
directory !== undefined
? join(directory, singleWorkerScriptPath)
: singleWorkerScriptPath;
usingWorkerScript = existsSync(scriptPath);

if (!usingWorkerScript) {
logger.log("No functions. Shimming...");
scriptPath = resolve(getBasePath(), "templates/pages-shim.ts");
} else {
const runBuild = async () => {
try {
await esbuild.build({
entryPoints: [scriptPath],
write: false,
plugins: [blockWorkerJsImports],
});
} catch {}
};
await runBuild();
watch([scriptPath], {
persistent: true,
ignoreInitial: true,
}).on("all", async () => {
await runBuild();
});
}
logger.log("No functions. Shimming...");
scriptPath = resolve(getBasePath(), "templates/pages-shim.ts");
}

await scriptReadyPromise;
Expand Down