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

_worker.js/ directory support in Pages #2966

Merged
merged 2 commits into from May 5, 2023
Merged

_worker.js/ directory support in Pages #2966

merged 2 commits into from May 5, 2023

Conversation

GregBrimble
Copy link
Member

@GregBrimble GregBrimble commented Mar 30, 2023

What this PR solves / how to test:

This PR adds support for an _worker.js/ directory in Pages. It adds support for:

  • wrangler pages dev
  • wrangler pages functions build
  • wrangler pages publish

The entrypoint (_worker.js/index.js) is bundled, and the rest of the files in the _worker.js/ directory are uploaded with the default rules + all .js files are treated as ESM.

This will be treated as an undocumented API for the time being.

Author has included the following, where applicable:

Reviewer has performed the following, where applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2023

🦋 Changeset detected

Latest commit: 6853fb7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Mar 31, 2023

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4862724294/npm-package-wrangler-2966

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/2966/npm-package-wrangler-2966

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4862724294/npm-package-wrangler-2966 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4862724294/npm-package-cloudflare-pages-shared-2966

Note that these links will no longer work once the GitHub Actions artifact expires.

@GregBrimble GregBrimble force-pushed the no-bundle-pages branch 2 times, most recently from eff07ee to 84a6fa4 Compare April 5, 2023 10:25
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #2966 (6853fb7) into main (0a77990) will increase coverage by 0.87%.
The diff coverage is 77.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2966      +/-   ##
==========================================
+ Coverage   73.54%   74.42%   +0.87%     
==========================================
  Files         167      168       +1     
  Lines       10499    10624     +125     
  Branches     2807     2855      +48     
==========================================
+ Hits         7722     7907     +185     
+ Misses       2777     2717      -60     
Impacted Files Coverage Δ
packages/wrangler/src/pages/dev.ts 18.14% <9.09%> (-0.64%) ⬇️
packages/wrangler/src/dev/start-server.ts 67.07% <81.81%> (+1.46%) ⬆️
packages/wrangler/src/api/pages/publish.tsx 87.93% <92.30%> (-0.26%) ⬇️
packages/wrangler/src/api/dev.ts 59.34% <100.00%> (-0.88%) ⬇️
packages/wrangler/src/bundle.ts 93.21% <100.00%> (+0.06%) ⬆️
packages/wrangler/src/dev.tsx 85.12% <100.00%> (+0.12%) ⬆️
packages/wrangler/src/entry.ts 98.40% <100.00%> (ø)
packages/wrangler/src/pages/build.ts 68.08% <100.00%> (+1.05%) ⬆️
...ckages/wrangler/src/pages/functions/buildWorker.ts 68.53% <100.00%> (+3.98%) ⬆️

... and 17 files with indirect coverage changes

@GregBrimble GregBrimble force-pushed the no-bundle-pages branch 6 times, most recently from 7830ccd to 1966c9d Compare April 20, 2023 14:38
@GregBrimble GregBrimble force-pushed the no-bundle-pages branch 4 times, most recently from 0551501 to 1b16312 Compare April 20, 2023 23:16
@GregBrimble GregBrimble marked this pull request as ready for review April 21, 2023 00:03
@GregBrimble GregBrimble requested review from a team as code owners April 21, 2023 00:03
@GregBrimble GregBrimble changed the title WIP: proper no-bundle in Pages _worker.js/ directory support in Pages Apr 21, 2023
@GregBrimble GregBrimble force-pushed the no-bundle-pages branch 2 times, most recently from 4f632cb to 8a93d66 Compare April 21, 2023 00:34
Copy link
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Looks good to me 😄 (based on my almost nonexistent knowledge of the codebase 😅)

packages/wrangler/src/api/pages/publish.tsx Outdated Show resolved Hide resolved
packages/wrangler/src/dev/start-server.ts Outdated Show resolved Hide resolved
.changeset/cuddly-rules-rest.md Outdated Show resolved Hide resolved
async fetch(request, env) {
return new Response("Hello from _worker.js/index.js" + cat);
},
};`
Copy link
Contributor

Choose a reason for hiding this comment

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

.trim()

packages/wrangler/src/api/dev.ts Outdated Show resolved Hide resolved
packages/wrangler/src/api/pages/publish.tsx Outdated Show resolved Hide resolved
packages/wrangler/src/dev/use-esbuild.ts Outdated Show resolved Hide resolved
packages/wrangler/src/dev/use-esbuild.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pages/build.ts Outdated Show resolved Hide resolved
Comment on lines 219 to 276
if (lstatSync(workerScriptPath).isDirectory()) {
const entrypoint = resolve(join(workerScriptPath, "index.js"));

const traverseModuleGraphResult = await traverseModuleGraph(
{
file: entrypoint,
directory: resolve(workerScriptPath),
format: "modules",
moduleRoot: resolve(workerScriptPath),
},
[
{
type: "ESModule",
globs: ["**/*.js"],
},
]
);

const bundleResult = await buildRawWorker({
workerScriptPath: entrypoint,
bundle: false,
outdir,
directory: buildOutputDirectory,
local: false,
sourcemap,
watch,
betaD1Shims: d1Databases,
});

bundle = {
modules: (traverseModuleGraphResult?.modules ??
bundleResult?.modules) as BundleResult["modules"],
dependencies: (bundleResult?.dependencies ??
traverseModuleGraphResult?.dependencies) as BundleResult["dependencies"],
resolvedEntryPointPath: (bundleResult?.resolvedEntryPointPath ??
traverseModuleGraphResult?.resolvedEntryPointPath) as BundleResult["resolvedEntryPointPath"],
bundleType: (bundleResult?.bundleType ??
traverseModuleGraphResult?.bundleType) as BundleResult["bundleType"],
stop: bundleResult?.stop,
sourceMapPath: bundleResult?.sourceMapPath,
};
} else {
/**
* `buildRawWorker` builds `_worker.js`, but doesn't give us the bundle
* we want to return, which includes the external dependencies (like wasm,
* binary, text). Let's output that build result to memory and only write
* to disk once we have the final bundle
*/
bundle = await buildRawWorker({
workerScriptPath,
outdir,
directory: buildOutputDirectory,
local: false,
sourcemap,
watch,
betaD1Shims: d1Databases,
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like a lot of this is the same as pages/dev. Could it be pulled out into a common helper function?

packages/wrangler/src/pages/dev.ts Show resolved Hide resolved
@GregBrimble GregBrimble force-pushed the no-bundle-pages branch 2 times, most recently from b66bda9 to 045c3df Compare May 2, 2023 14:28
});

return {
modules: traverseModuleGraphResult?.modules ?? bundleResult?.modules ?? [],
Copy link
Contributor

@jahands jahands May 2, 2023

Choose a reason for hiding this comment

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

Under what case would it use bundleResult?.modules? Mind adding a comment explaining what is happening here? I'm not as familiar with esbuild/etc and the way we're doing the traversal and then bundling (but not bundling) is confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good shout. This was me copy-pasting from elsewhere, but you're right. It'll never fallback to bundleResults.

"wrangler": minor
---

feat: Add support for the undocumented `_worker.js/` directory in Pages
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be documented?

@GregBrimble GregBrimble merged commit e351afc into main May 5, 2023
11 checks passed
@GregBrimble GregBrimble deleted the no-bundle-pages branch May 5, 2023 01:01
@github-actions github-actions bot mentioned this pull request May 5, 2023
@penalosa penalosa mentioned this pull request May 5, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants