nodejs compat functions build #3133

dario-piotrowicz · 2023-05-03T17:31:17Z

Note continuation of #3100

Fixes cloudflare/next-on-pages#199

What this PR solves / how to test:

Fixes passing the nodejs_compat flag through to wrangler pages functions build
Also improve the error message so that for pages it doesn't ask to update wrangler.toml

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 · 2023-05-03T17:31:20Z

🦋 Changeset detected

Latest commit: dc560d9

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

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

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 · 2023-05-03T17:34:31Z

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/4892376248/npm-package-wrangler-3133

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4892376248/npm-package-wrangler-3133 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4892376248/npm-package-cloudflare-pages-shared-3133

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

Skye-31 · 2023-05-03T17:49:34Z

packages/wrangler/src/bundle.ts

+			const instructionForUser = `${
+				forPages
+					? 'Add the "nodejs_compat" compatibility flag to your Pages project'
+					: 'Add "node_compat = true" to your wrangler.toml file'


Might be misunderstanding the changes, but should this be compatibility_flags = ["nodejs_compat"]?
node_compat is the legacy polyfill implementation

@Skye-31 thanks a lot for the comment, I think you're right, I wasn't really really looking into updating that error message but just adding the Pages version (as you can see the non-pages error message is totally untouched)

I can also see that node_compat is used in a handful of places still so it might just be worth it to change all that in a followup PR instead and keep this one focused only on they Pages related changes?

Also I am not totally sure if here node_compat is used for a specific reason 🤷

Anyways I am happy to change it or keep it and do the cleanup at some other point 🙂

I think node_compat is just used here because the error message was written before the newer nodejs_compat work. Might be worth updating in a future PR!

I just checked the "handful of places" I mentioned above and it seems like the only place that needs fixing is the error message I changed here 😓 🤦 (sorry should have checked the search result more carefully the first time around)

I'll open a PR to fix that 🙂

penalosa · 2023-05-03T19:29:38Z

packages/wrangler/src/pages/functions/buildWorker.ts

@@ -222,6 +222,7 @@ export function buildRawWorker({
 			targetConsumer: local ? "dev" : "publish",
 			local,
 			experimentalLocal: false,
+			forPages: true,


Is this the only place Pages calls bundleWorker?

no sorry, I missed a couple of places 😓, I've added the option there as well 🙏

codecov · 2023-05-03T19:49:04Z

Codecov Report

Merging #3133 (dc560d9) into main (e351afc) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3133      +/-   ##
==========================================
+ Coverage   74.83%   74.87%   +0.04%     
==========================================
  Files         179      179              
  Lines       10852    10858       +6     
  Branches     2877     2880       +3     
==========================================
+ Hits         8121     8130       +9     
+ Misses       2731     2728       -3

Impacted Files	Coverage Δ
packages/wrangler/src/pages/build.ts	`68.08% <ø> (ø)`
...ckages/wrangler/src/pages/functions/buildPlugin.ts	`19.35% <ø> (ø)`
...ckages/wrangler/src/pages/functions/buildWorker.ts	`68.53% <ø> (ø)`
packages/wrangler/src/bundle.ts	`93.39% <100.00%> (+0.17%)`	⬆️

... and 2 files with indirect coverage changes

dario-piotrowicz · 2023-05-04T21:11:08Z

Local testing, wrangler pages publish before and after:

dario-piotrowicz · 2023-05-04T22:30:00Z

Local testing, wrangler pages dev (without the compatibility flag), no difference between before and after:

dario-piotrowicz · 2023-05-04T22:35:07Z

Local testing, wrangler pages functions build (without the compatibility flag), before and after:

packages/wrangler/src/bundle.ts

GregBrimble · 2023-05-04T22:59:21Z

Can you cut a new issue to track exposing this error properly in dev too, please?

GregBrimble

Would be good to test, but not the end of the world.

dario-piotrowicz · 2023-05-04T23:15:13Z

@GregBrimble #3147 🙂👍

Co-authored-by: Greg Brimble <developer@gregbrimble.com>

dario-piotrowicz · 2023-05-05T09:46:42Z

Would be good to test, but not the end of the world.

@GregBrimble I've added a check in functions-build.test.ts 🙂👍

GregBrimble · 2023-05-05T09:47:57Z

Perfect, thank you!

penalosa

Approved, after discussion in-person. I'm slightly concerned about the precedent this sets (bundling being aware of the target), but I can't think of a better solution right now.

Longer term, @dario-piotrowicz could you make a ticket to shift our NodeJS module erroring into an esbuild plugin that can be different between Pages and Workers, and injected into the build process without bundling having to be aware of the source?

dario-piotrowicz · 2023-05-05T15:09:40Z

@penalosa sure thanks! 😄👍

dario-piotrowicz · 2023-05-05T15:37:35Z

@penalosa I've created this issue: #3155

Please have a look at it and feel free to tweak it if needed 🙂

dario-piotrowicz requested review from a team as code owners May 3, 2023 17:31

Skye-31 reviewed May 3, 2023

View reviewed changes

penalosa reviewed May 3, 2023

View reviewed changes

dario-piotrowicz force-pushed the dario/nodejs-compat-functions-build branch from b5bd7ff to b9cb66c Compare May 4, 2023 14:24

GregBrimble reviewed May 4, 2023

View reviewed changes

packages/wrangler/src/bundle.ts Outdated Show resolved Hide resolved

GregBrimble approved these changes May 4, 2023

View reviewed changes

dario-piotrowicz mentioned this pull request May 4, 2023

🐛 BUG: wrangler pages dev doesn't provide a user-friendly error message on node imports #3147

Closed

james-elicx mentioned this pull request May 5, 2023

13.4, Server Actions and Turbopack cloudflare/next-on-pages#218

Open

3 tasks

penalosa and others added 6 commits May 5, 2023 10:37

Pass through nodejs compat to pages functions build

fb91ac7

improve error message for nodejs_compat missing for pages build

dae8dd8

add forPages option to all pages bundleWorker calls

48a8230

run prettify

5ac5b00

Update packages/wrangler/src/bundle.ts

91d20b5

Co-authored-by: Greg Brimble <developer@gregbrimble.com>

update tests

52f9745

dario-piotrowicz force-pushed the dario/nodejs-compat-functions-build branch from 78d4dfd to 52f9745 Compare May 5, 2023 09:45

dario-piotrowicz added 2 commits May 5, 2023 10:51

prettify code

cbffa21

add changeset

dc560d9

dario-piotrowicz force-pushed the dario/nodejs-compat-functions-build branch from d51d8a4 to dc560d9 Compare May 5, 2023 10:21

GregBrimble approved these changes May 5, 2023

View reviewed changes

dario-piotrowicz mentioned this pull request May 5, 2023

[🐛 Bug]: Nextjs v13.3.1 fresh install worker threw exception cloudflare/next-on-pages#199

Closed

1 task

penalosa approved these changes May 5, 2023

View reviewed changes

GregBrimble merged commit d078800 into cloudflare:main May 5, 2023
11 checks passed

dario-piotrowicz deleted the dario/nodejs-compat-functions-build branch May 5, 2023 15:14

github-actions bot mentioned this pull request May 5, 2023

Version Packages #3149

Merged

dario-piotrowicz mentioned this pull request May 5, 2023

🚀 Improvement: create an esbuild plugin that can differentiate between pages and workers #3155

Open

dario-piotrowicz mentioned this pull request May 8, 2023

Update README.md cloudflare/next-on-pages#223

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nodejs compat functions build #3133

nodejs compat functions build #3133

dario-piotrowicz commented May 3, 2023

changeset-bot bot commented May 3, 2023 •

edited

github-actions bot commented May 3, 2023 •

edited

Skye-31 May 3, 2023 •

edited

dario-piotrowicz May 3, 2023

penalosa May 3, 2023

dario-piotrowicz May 5, 2023

penalosa May 3, 2023

dario-piotrowicz May 3, 2023

codecov bot commented May 3, 2023 •

edited

dario-piotrowicz commented May 4, 2023

dario-piotrowicz commented May 4, 2023

dario-piotrowicz commented May 4, 2023

GregBrimble commented May 4, 2023

GregBrimble left a comment

dario-piotrowicz commented May 4, 2023

dario-piotrowicz commented May 5, 2023

GregBrimble commented May 5, 2023

penalosa left a comment

dario-piotrowicz commented May 5, 2023

dario-piotrowicz commented May 5, 2023

nodejs compat functions build #3133

nodejs compat functions build #3133

Conversation

dario-piotrowicz commented May 3, 2023

changeset-bot bot commented May 3, 2023 • edited

🦋 Changeset detected

github-actions bot commented May 3, 2023 • edited

Skye-31 May 3, 2023 • edited

Choose a reason for hiding this comment

dario-piotrowicz May 3, 2023

Choose a reason for hiding this comment

penalosa May 3, 2023

Choose a reason for hiding this comment

dario-piotrowicz May 5, 2023

Choose a reason for hiding this comment

penalosa May 3, 2023

Choose a reason for hiding this comment

dario-piotrowicz May 3, 2023

Choose a reason for hiding this comment

codecov bot commented May 3, 2023 • edited

Codecov Report

dario-piotrowicz commented May 4, 2023

dario-piotrowicz commented May 4, 2023

dario-piotrowicz commented May 4, 2023

GregBrimble commented May 4, 2023

GregBrimble left a comment

Choose a reason for hiding this comment

dario-piotrowicz commented May 4, 2023

dario-piotrowicz commented May 5, 2023

GregBrimble commented May 5, 2023

penalosa left a comment

Choose a reason for hiding this comment

dario-piotrowicz commented May 5, 2023

dario-piotrowicz commented May 5, 2023

changeset-bot bot commented May 3, 2023 •

edited

github-actions bot commented May 3, 2023 •

edited

Skye-31 May 3, 2023 •

edited

codecov bot commented May 3, 2023 •

edited