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

Added --node-compat to Pages Dev & Functions #1000

Merged
merged 1 commit into from
May 24, 2022
Merged

Conversation

JacobMGEvans
Copy link
Contributor

@JacobMGEvans JacobMGEvans commented May 13, 2022

pages dev <dir> & wrangler pages functions build will have a --node-compat flag powered by @esbuild-plugins/node-globals-polyfill (which in itself is powered by rollup-plugin-node-polyfills). The only difference in pages will be it does not check the wrangler.toml so the node_compat = truewill not enable it for wrangler pages functionality.

resolves #890

@JacobMGEvans JacobMGEvans added pages Relating to Pages polish Small improvements to the experience labels May 13, 2022
@changeset-bot
Copy link

changeset-bot bot commented May 13, 2022

🦋 Changeset detected

Latest commit: 3eb04d4

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
Copy link
Contributor

github-actions bot commented May 13, 2022

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

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2374149528/npm-package-wrangler-1000

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

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1000/npm-package-wrangler-1000

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2374149528/npm-package-wrangler-1000 dev path/to/script.js

@JacobMGEvans JacobMGEvans force-pushed the pages-node-compat branch 5 times, most recently from 9201fc0 to 89ef9a5 Compare May 13, 2022 21:43
@GregBrimble
Copy link
Member

Really cool to see this working, but I don't think we can merge until we have a way for people to use this in production. So that'll require either a project-level setting in the dashboard, or a Pages config file. I imagine probably the former, but the Pages team will need to confirm.

@JacobMGEvans
Copy link
Contributor Author

Without --node-compat running pages dev
without node-compat 2022-05-13 at 3 19 06 PM

Stripe --node-compat in pages Function:
with node-compat 2022-05-13 at 3 19 34 PM

Route for pages Function:
route of stripe Function 2022-05-13 at 3 19 51 PM

@JacobMGEvans
Copy link
Contributor Author

JacobMGEvans commented May 13, 2022

Really cool to see this working, but I don't think we can merge until we have a way for people to use this in production. So that'll require either a project-level setting in the dashboard, or a Pages config file. I imagine probably the former, but the Pages team will need to confirm.

@GregBrimble Awesome let me know what ya'll decide next week. I might need help with this one failing test anyway lol not sure why the plugin import static asset was affected by these changes
Screen Shot 2022-05-13 at 4 50 32 PM

@WalshyDev
Copy link
Member

Stripe --node-compat in pages Function: with node-compat 2022-05-13 at 3 19 34 PM

You're the best Jacob!

@threepointone
Copy link
Contributor

So cool. Looking forward to seeing this in Pages.

@threepointone threepointone changed the title feat: Added --node-compat to Pages Dev & Functions DO NOT LAND feat: Added --node-compat to Pages Dev & Functions May 15, 2022
@JacobMGEvans JacobMGEvans added blocked Blocked on other work and removed blocked Blocked on other work labels May 16, 2022
@JacobMGEvans JacobMGEvans requested a review from a team as a code owner May 19, 2022 20:45
@JacobMGEvans JacobMGEvans changed the title DO NOT LAND feat: Added --node-compat to Pages Dev & Functions Added --node-compat to Pages Dev & Functions May 19, 2022
@JacobMGEvans JacobMGEvans force-pushed the pages-node-compat branch 7 times, most recently from fd601ae to 8104a17 Compare May 20, 2022 18:17
@JacobMGEvans JacobMGEvans force-pushed the pages-node-compat branch 22 times, most recently from a84d615 to 481c444 Compare May 23, 2022 21:50
`pages dev <dir>` & `wrangler pages functions build` will have a `--node-compat` flag powered by @esbuild-plugins/node-globals-polyfill (which in itself is powered by rollup-plugin-node-polyfills). The only difference in `pages` will be it does not check the `wrangler.toml` so the `node_compat = true`
will not enable it for `wrangler pages` functionality.

resolves #890
@@ -1579,6 +1601,12 @@ export const pages: BuilderCallback<unknown, unknown> = (yargs) => {
type: "string",
description: "The directory to output static assets to",
},
"node-compat": {
describe: "Enable node.js compaitibility",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe: "Enable node.js compaitibility",
describe: "Enable node.js compatibility",

@@ -1299,6 +1303,12 @@ export const pages: BuilderCallback<unknown, unknown> = (yargs) => {
default: false,
description: "Auto reload HTML pages when change is detected",
},
"node-compat": {
describe: "Enable node.js compaitibility",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe: "Enable node.js compaitibility",
describe: "Enable node.js compatibility",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make another PR, didn't see these comments on my phone.

@@ -1013,7 +1017,7 @@ const createDeployment: CommandModule<
let builtFunctions: string | undefined = undefined;
const functionsDirectory = join(cwd(), "functions");
if (existsSync(functionsDirectory)) {
const outfile = join(tmpdir(), "./functionsWorker.js");
const outfile = join(tmpdir(), `./functionsWorker-${Math.random()}.js`);
Copy link
Contributor

Choose a reason for hiding this comment

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

@GregBrimble - these changes are due to the tmpdir() function always returning the same folder, which resulted in parallel tests overwriting each other's files.

We should come up with a better solution than actually needing to write these files to disk, ideally.

Also we could consider using fs.mkdtempSync() in combination with tmpdir() to really encapsulate separate runs of Pages builds.

But for now I think this is a reasonable way to unblock this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pages Relating to Pages polish Small improvements to the experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --node-compat to Pages Functions
5 participants