replace process.env.NODE_ENV at build time and enable react module-resolution build test

[dario-piotrowicz]

resolves #82

[pkg-pr-new]

Open in Stackblitz

npm i https://pkg.pr.new/flarelabs-net/vite-plugin-cloudflare/@flarelabs-net/vite-plugin-cloudflare@94

commit: 9b298ee

[jamesopstad]

According to this comment, Vite replaces process.env.NODE_ENV when keepProcessEnv is false.

https://discord.com/channels/804011606160703521/1314204727621193788/1314404199747813446

I'm wondering if it would be better to only set keepProcessEnv: true when nodejs_compat is enabled?

[dario-piotrowicz]

According to this comment, Vite replaces process.env.NODE_ENV when keepProcessEnv is false.

https://discord.com/channels/804011606160703521/1314204727621193788/1314404199747813446

I'm wondering if it would be better to only set keepProcessEnv: true when nodejs_compat is enabled?

Sorry I am not sure I follow

Specifically thinking of the react use case, we want react to work both with and without nodejs_compat, but in either case it requires process.env.NODE_ENV to be replaced (either by us or vite), right? so I think that either way we do need the replacement introduced in this PR? are you suggesting to add the conditional setting of keepProcessEnv on top of the current changes? in that case what would the gain be?
Or am I missing something? 🤔

[jamesopstad]

Ah, I probably misunderstood. I thought this example worked if nodejs_compat was enabled. But I'm guessing you mean that process.env.NODE_ENV doesn't exist even if nodejs_compat is enabled, in which case you're right.

It makes more sense to me now why this is a Cloudflare specific issue rather than a more general Vite one.

[dario-piotrowicz]

Ah, I probably misunderstood. I thought this example worked if nodejs_compat was enabled.

I don't actually know, I'm unable to see if this works with nodejs_compat since when I try to set the flag I get the following error:

Cannot find module 'unenv/runtime/node/process/$cloudflare' imported from '/Users/dario/Repos/vite-plugin-cloudflare/playground-temp/module-resolution/src/index.ts

But I'm guessing you mean that process.env.NODE_ENV doesn't exist even if nodejs_compat is enabled, in which case you're right.

Ah, sorry I see what you meant now, you thought that process.env.NODE_ENV could be just present with nodejs_compat? I don't think it is:

maybe it should? WDYT? @petebacondarwin?

[jamesopstad]

I think your approach is correct. Even if process.env.NODE_ENV was available we'd want it to be statically replaced so that people don't end up with development builds of libraries (e.g. React) in production builds of their workers. It's just an empty object in production though - https://developers.cloudflare.com/workers/runtime-apis/nodejs/process/#processenv.

I sort of wonder if we should actually always be using keepProcessEnv: false? Would this work or would it break other things in user code?

Let's check with @petebacondarwin for approval.

[dario-piotrowicz]

I think your approach is correct. Even if process.env.NODE_ENV was available we'd want it to be statically replaced so that people don't end up with development builds of libraries (e.g. React) in production builds of their workers.

oh yeah! treeshaking slipped my mind for a sec! 😓 👍

[jamesopstad]

@petebacondarwin @dario-piotrowicz Please can you explain what would happen in our case if we always had keepProcessEnv: false and how that relates to this - https://developers.cloudflare.com/workers/runtime-apis/nodejs/process/#processenv.

[dario-piotrowicz]

@petebacondarwin @dario-piotrowicz Please can you explain what would happen in our case if we always had keepProcessEnv: false and how that relates to this - https://developers.cloudflare.com/workers/runtime-apis/nodejs/process/#processenv.

This is, as far as I can tell, the only place in which keepProcessEnv is currently used: ...(keepProcessEnv ? {} : processEnv), (alongside the upcoming changes in vitejs/vite#18899), so all it does is adding these defines:

Object.assign(processEnv, {
  'process.env': `{}`,
  'global.process.env': `{}`,
  'globalThis.process.env': `{}`,
  'process.env.NODE_ENV': JSON.stringify(nodeEnv),
  'global.process.env.NODE_ENV': JSON.stringify(nodeEnv),
  'globalThis.process.env.NODE_ENV': JSON.stringify(nodeEnv),
})

(source)

As per the Vite docs: Define global constant replacements. Entries will be defined as globals during dev and statically replaced during build

My understanding is that keepProcess: false would cause all the defines listed above to be replaced/controlled by Vite (with some global constants in dev and with static replacements in build) meaning that workerd's process.env would be completely out of the picture, both for dev but especially in production.

So I think that basically with keepProcess: false would completely cause workerd's process.env to be overshadowed by vite's own process.env implementation

[dario-piotrowicz]

@petebacondarwin @dario-piotrowicz Please can you explain what would happen in our case if we always had keepProcessEnv: false and how that relates to this - https://developers.cloudflare.com/workers/runtime-apis/nodejs/process/#processenv.

This is, as far as I can tell, the only place in which keepProcessEnv is currently used: ...(keepProcessEnv ? {} : processEnv), (alongside the upcoming changes in vitejs/vite#18899), so all it does is adding these defines:

Object.assign(processEnv, {
  'process.env': `{}`,
  'global.process.env': `{}`,
  'globalThis.process.env': `{}`,
  'process.env.NODE_ENV': JSON.stringify(nodeEnv),
  'global.process.env.NODE_ENV': JSON.stringify(nodeEnv),
  'globalThis.process.env.NODE_ENV': JSON.stringify(nodeEnv),
})

(source)

As per the Vite docs: Define global constant replacements. Entries will be defined as globals during dev and statically replaced during build

My understanding is that keepProcess: false would cause all the defines listed above to be replaced/controlled by Vite (with some global constants in dev and with static replacements in build) meaning that workerd's process.env would be completely out of the picture, both for dev but especially in production.

So I think that basically with keepProcess: false would completely cause workerd's process.env to be overshadowed by vite's own process.env implementation

(👆 if you want I can spin up a new project and double check/corroborate the above)

[jamesopstad]

I did just try setting keepProcessEnv: false and running all the tests. They all passed (including node-compat/worker-process). So either something else is going on here or we don't have test coverage for this.

This also meant I could enable the React module-resolution test and it passes without any changes.

[dario-piotrowicz]

I did just try setting keepProcessEnv: false and running all the tests. They all passed (including node-compat/worker-process). So either something else is going on here or we don't have test coverage for this.

yeah I suspect that we don't have test coverage for this 👍

I'll have a quick look

[dario-piotrowicz]

I did just try setting keepProcessEnv: false and running all the tests. They all passed (including node-compat/worker-process). So either something else is going on here or we don't have test coverage for this.

yeah I suspect that we don't have test coverage for this 👍

I'll have a quick look

I had a quick look and I noticed that I actually misread the docs (sorry totally my bad) 😓

in workers process.env is an empty object, so I think it's actually ok to just use keepProcessEnv: false since Vite replaces existing process.env with empty objects, so there is actually no real difference right?

keepProcessEnv: true could be needed if the workerd process.env workerd implementation were to change at some point, but yeah right not keepProcessEnv: false might be appropriate 👍

[dario-piotrowicz]

closing in favour of #107 (since the core of the PR changes I figured it would make sense to just open a new one, I hope you guys done mind 😅)