Please remove `process.env.NODE_ENV` in `dist/immer.mjs`

[diandian18]

🚀 Feature Proposal

I use import { produce } from 'immer' in vite project. After building, process.env.NODE_ENV is still in my built file.

Motivation

Could process.env.NODE_ENV be removed in dist/immer.mjs?

[markerikson]

That inclusion is intentional, and Vite should be replacing it automatically.

[blixt]

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.

[markerikson]

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.

[blixt]

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.:

{
  "exports": {
    "node": "./dist/cjs/index.js",
    "default": "./dist/esm/index.js"
  }
}

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 exports to 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.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.

[guybedford]

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:

{
  "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 browser entry to this in the above "exports" would be very helpful if that is the intention.

[markerikson]

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]

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.

[markerikson]

@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]

@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.

I understand that you refer to what you said earlier?

if you need to use it in a browser directly you should be pointing it to dist/immer.production.mjs

If so, I believe that the fix is to add a browser conditional 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-runner and other tools which recognise the nonstandard browser condition.