You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Please remove process.env.NODE_ENV in dist/immer.mjs #1141
This problem is not only about Vite, and this is typically not happening with other packages. As an example, using immer via JSPM (e.g. using the Import Map Generator) will not work in a browser unless you shim a Node.js environment.
I'm not familiar with how JSPM resolves which file to use inside of an NPM package, but if you need to use it in a browser directly you should be pointing it to dist/immer.production.mjs, which is an ESM module that has already been precompiled to strip out the process.env.NODE_ENV checks, minified, and is meant for use as a modern <script type="module"> directly.
JSPM is an import maps first approach to package management so it does support development builds and production builds, so the true solution might not be as simple as to just say "use production builds if you want the Node environment stuff stripped out". However, I believe JSPM will respect the exports conditional property, i.e.:
Now like I implied initially, this would hide the development version from JSPM (which would be useful if you want to have a bundler-less browser first environment with development support, like we did at Framer which is completely ES Modules + Import Maps based). But from what I can see, there is no ./dist/immer.browser.mjs or so which would be needed, and I do think the change I wrote above would already be a step up.
Now I'm not sure what @guybedford's stance is on these kinds of situations – for example esm.sh appears to have taken another approach where it doesn't include the process reference – possibly by figuring out which module is for production (maybe with a manual override?), or by just post-processing the code to consider process.env.NODE_ENV a constant that can be turned into true or false.
JSPM will replace process.env.NODE_ENV in CommonJS as a CommonJS feature contract, but yes for native ES modules we don't do the same thing, the reason being that we don't consider it to be a modern pattern going forward into the native ES ecosystem.
What we do for CommonJS is effectively build two files - one with the dev and one with the production condition, and then set up entry conditions to those variations depending on which mode the package is loaded. It's a little messy, and doing this explicitly at the package level is the preferred model (using the "development" and "production" package conditions - https://nodejs.org/docs/latest/api/packages.html#community-conditions-definitions).
This is mostly a principled stance in JSPM though which we can change anytime, so if there is strong resistance to moving away from process.env.NODE_ENV in the ESM ecosystem we could support it in JSPM for native modules. import.meta.env is also another active discussion as a native ESM approach in Node.js itself.
As for using dist/immer.production.mjs, this isn't described in any of the package.json entry points for tools to pick up:
Sure, makes sense. Immer isn't my package, but I did contribute the packaging setup based on what I was working on for the Redux libraries. File a PR here?
That inclusion is intentional, and Vite should be replacing it automatically.
To chime in, the usage of process is also blocking me from moving from v9 to v10 because I run tests in browser using @web/test-runner which tries to be as "browser native" as possible.
@tpluscode how does that tool determine which package artifact to use?
It does say "powered by ESBuild", so I assume it's finding the standard ESM artifact, but as discussed here that artifact is intentionally not meant for use directly in a browser.
@web/test-runner is not really bundling. It is meant to be as fast and as close to the original sources as possible. It can compile CJS and dedupe imports but that's about it. Like some ESM CDNs, I suppose, but locally.
It does also have the option to create custom transform functions where I could get rid of process. I am actually doing something similar for version 9 right now but should hope that that should not be necessary.
as discussed here that artifact is intentionally not meant for use directly in a browser.
Activity
markerikson commentedon Sep 4, 2024
That inclusion is intentional, and Vite should be replacing it automatically.
blixt commentedon Oct 3, 2024
This problem is not only about Vite, and this is typically not happening with other packages. As an example, using
immervia JSPM (e.g. using the Import Map Generator) will not work in a browser unless you shim a Node.js environment.markerikson commentedon Oct 4, 2024
I'm not familiar with how JSPM resolves which file to use inside of an NPM package, but if you need to use it in a browser directly you should be pointing it to
dist/immer.production.mjs, which is an ESM module that has already been precompiled to strip out theprocess.env.NODE_ENVchecks, minified, and is meant for use as a modern<script type="module">directly.blixt commentedon Oct 4, 2024
JSPM is an import maps first approach to package management so it does support development builds and production builds, so the true solution might not be as simple as to just say "use production builds if you want the Node environment stuff stripped out". However, I believe JSPM will respect the
exportsconditional property, i.e.:You can read more here: https://nodejs.org/api/packages.html#conditional-exports
Basically this would be the most official way to say "for Node environments we use this entry point, otherwise we use this other one".
For Immer, which already uses
exportsto differentiate CJS and ESM it would look something like this:{ // ... "exports": { "./package.json": "./package.json", ".": { "types": "./dist/immer.d.ts", "node": { "import": "./dist/immer.mjs", "require": "./dist/cjs/index.js" }, "default": "./dist/immer.production.mjs" } }, // ... }Now like I implied initially, this would hide the development version from JSPM (which would be useful if you want to have a bundler-less browser first environment with development support, like we did at Framer which is completely ES Modules + Import Maps based). But from what I can see, there is no
./dist/immer.browser.mjsor so which would be needed, and I do think the change I wrote above would already be a step up.Now I'm not sure what @guybedford's stance is on these kinds of situations – for example esm.sh appears to have taken another approach where it doesn't include the
processreference – possibly by figuring out which module is for production (maybe with a manual override?), or by just post-processing the code to considerprocess.env.NODE_ENVa constant that can be turned intotrueorfalse.guybedford commentedon Oct 6, 2024
JSPM will replace
process.env.NODE_ENVin CommonJS as a CommonJS feature contract, but yes for native ES modules we don't do the same thing, the reason being that we don't consider it to be a modern pattern going forward into the native ES ecosystem.What we do for CommonJS is effectively build two files - one with the dev and one with the production condition, and then set up entry conditions to those variations depending on which mode the package is loaded. It's a little messy, and doing this explicitly at the package level is the preferred model (using the
"development"and"production"package conditions - https://nodejs.org/docs/latest/api/packages.html#community-conditions-definitions).This is mostly a principled stance in JSPM though which we can change anytime, so if there is strong resistance to moving away from
process.env.NODE_ENVin the ESM ecosystem we could support it in JSPM for native modules.import.meta.envis also another active discussion as a native ESM approach in Node.js itself.As for using
dist/immer.production.mjs, this isn't described in any of the package.json entry points for tools to pick up:{ "main": "./dist/cjs/index.js", "module": "./dist/immer.legacy-esm.js", "exports": { "./package.json": "./package.json", ".": { "types": "./dist/immer.d.ts", "import": "./dist/immer.mjs", "require": "./dist/cjs/index.js" } }, "jsnext:main": "dist/immer.mjs", "react-native": "./dist/immer.legacy-esm.js" }Adding a
browserentry to this in the above"exports"would be very helpful if that is the intention.markerikson commentedon Oct 6, 2024
Sure, makes sense. Immer isn't my package, but I did contribute the packaging setup based on what I was working on for the Redux libraries. File a PR here?
tpluscode commentedon Nov 28, 2024
To chime in, the usage of
processis also blocking me from moving from v9 to v10 because I run tests in browser using @web/test-runner which tries to be as "browser native" as possible.markerikson commentedon Nov 28, 2024
@tpluscode how does that tool determine which package artifact to use?
It does say "powered by ESBuild", so I assume it's finding the standard ESM artifact, but as discussed here that artifact is intentionally not meant for use directly in a browser.
Dug through the source and found https://github.com/modernweb-dev/web/blob/17cfc0d70f46b321912e4506b2cccae1b16b1534/packages/dev-server-esbuild/src/EsbuildPlugin.ts as a likely-looking ESBuild config, and it looks like it's not doing anything to replace
process.env.NODE_ENV.Seems like an impedance mismatch - using a bundler, but not doing a full transform.
tpluscode commentedon Dec 5, 2024
@web/test-runneris not really bundling. It is meant to be as fast and as close to the original sources as possible. It can compile CJS and dedupe imports but that's about it. Like some ESM CDNs, I suppose, but locally.It does also have the option to create custom transform functions where I could get rid of
process. I am actually doing something similar for version 9 right now but should hope that that should not be necessary.I understand that you refer to what you said earlier?
If so, I believe that the fix is to add a
browserconditional to the existing exports"exports": { "./package.json": "./package.json", ".": { "types": "./dist/immer.d.ts", "import": "./dist/immer.mjs", + "browser": "./dist/immer.production.mjs", "require": "./dist/cjs/index.js" } },I believe that would allow tool like
@web/test-runnerand other tools which recognise the nonstandardbrowsercondition.