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

feat: set node: true on cloudflare-pages preset #1315

Closed
wants to merge 1 commit into from

Conversation

Smef
Copy link

@Smef Smef commented Jun 16, 2023

πŸ”— Linked issue

#1314

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Cloudflare pages supports node by default, but the cloudflare-pages.ts preset has this option set to false as it is inherited from the base preset. This PR adds an explicit node: true to the cloudflare-pages preset.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@Hebilicious
Copy link
Member

@Smef I am a little bit confused by this propose change.

You're saying in the issue that CFP supports node, but this is for the build, not for the runtime. The runtime isn't node, it's workerd, which has a nodejs_compat optional flag, but it's not the same as being a node environment.

@Smef
Copy link
Author

Smef commented Jun 17, 2023

I’m talking about the Cloudflare Pages build environment, which is different than what you linked to. https://developers.cloudflare.com/pages/platform/language-support-and-tools/

It looks like your link was to Cloudflare Workers info, which isn’t the same thing. It’s quite possible that this PR is for the wrong preset, but the issue this is for is related to the CFP build environment which is full Node. Am I misunderstanding this preset?

@Hebilicious
Copy link
Member

Hebilicious commented Jun 17, 2023

I’m talking about the Cloudflare Pages build environment, which is different than what you linked to. developers.cloudflare.com/pages/platform/language-support-and-tools

It looks like your link was to Cloudflare Workers info, which isn’t the same thing. It’s quite possible that this PR is for the wrong preset, but the issue this is for is related to the CFP build environment which is full Node. Am I misunderstanding this preset?

Both workers and pages are built on the same technology, workerd, which is a Javascript runtime that is different from node.js. Cloudflare Pages CI (ie the thing that hooks to a github repo and automatically rebuilds your website) uses Node for your builds, but not the runtime.

@Smef
Copy link
Author

Smef commented Jun 17, 2023

Maybe you can point me in the right direction if this isn't it, or if this isn't the correct preset configuration option, but the CFB build environment is what I'm talking about, and it runs actual Node. The issue is that currently when running in the Cloudflare Pages build environment, either v1 or the v2 beta (as described here) and deploying a static site, Nitro incorrectly things that Node isn't available.

The build environment and build process give messages like

02:04:51.263 | Attempting node version '18' from .nvmrc
-- | --
02:04:52.312 | v18.16.0 is already installed.
02:04:53.762 | Now using node v18.16.0 (npm v9.5.1)

It runs the npx commands here as well as part of the one-time build process. In this environment it appears to be Node, and not workerd, which matches the documentation for the platform.

Is the issue here that this preset configuration is not for the CFP build environment, but is for the runtime environment, and this PR is just for the wrong file? Again, the environment I'm trying to address here is definitely running Node and only during a single-run build step, not any sort of deployed runtime environment, and Nitro is guessing the node status incorrectly (see this issue for Nuxt/Image and IPX as an example.)

@Hebilicious
Copy link
Member

Maybe you can point me in the right direction if this isn't it, or if this isn't the correct preset configuration option, but the CFB build environment is what I'm talking about, and it runs actual Node. The issue is that currently when running in the Cloudflare Pages build environment, either v1 or the v2 beta (as described here) and deploying a static site, Nitro incorrectly things that Node isn't available.

The build environment and build process give messages like

02:04:51.263 | Attempting node version '18' from .nvmrc
-- | --
02:04:52.312 | v18.16.0 is already installed.
02:04:53.762 | Now using node v18.16.0 (npm v9.5.1)

It runs the npx commands here as well as part of the one-time build process. In this environment it appears to be Node, and not workerd, which matches the documentation for the platform.

Is the issue here that this preset configuration is not for the CFP build environment, but is for the runtime environment, and this PR is just for the wrong file? Again, the environment I'm trying to address here is definitely running Node and only during a single-run build step, not any sort of deployed runtime environment, and Nitro is guessing the node status incorrectly (see this issue for Nuxt/Image and IPX as an example.)

My apologies, now that I see the Image issue I see your problem.

Looking into it, this node: true flag is for the rollup configuration, and it looks like it does affect the final bundle. So I'm not sure this is a sane default for all builds...

Given that Nitro can only be built with Node, this issue shouldn't happen in the first place though.

@Smef
Copy link
Author

Smef commented Jun 17, 2023

As a test of this, running node -v npm -v and which node commands in the build environment gives the following results:

17:02:29.619	v18.16.0
17:02:30.023	9.7.1
17:02:30.037	/opt/buildhome/.nvm/versions/node/v18.16.0/bin/node

Nitro definitely seems to be detecting (or assuming?) this incorrectly, and reading nitro.options.node returns false in this environment, which doesn't seem to be correct.

@Smef
Copy link
Author

Smef commented Jun 17, 2023

Ah, so is your thought here that this just isn't the right fix for this issue, and the problem needs to be addressed elsewhere?

@Hebilicious
Copy link
Member

Ah, so is your thought here that this just isn't the right fix for this issue, and the problem needs to be addressed elsewhere?

Yes, this could cause a bunch of others issues as doing this disable a lot of the unenv mocks. However I'm very often getting issues with this part of the code anyways, see here nuxt/nuxt#21619

@pi0
Copy link
Member

pi0 commented Jun 18, 2023

Hi. Thanks for the PR. Following up conversation, @Hebilicious is correct. Nuxt nitro always uses Node.js for building phase. The node: true flag toggles unenv polyfill levels which are needed to add compatibility layer to cloudflare workers/pages which both are not a Node.js environment.

A related matter, cloudflare workers, has some native polyfills for some node.js imports which we need to enable once the compatibility flag is enabled by users (which is not by default). #1171

@pi0 pi0 closed this Jun 18, 2023
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.

3 participants