Published NPM code references process.env.NODE_ENV

[cuberoot]

I am trying out rc1 and I hit an issue where browser code is trying to access process.env.NODE_ENV.

This code gets built into:

        if (process.env.NODE_ENV !== "production") {

I hit this when importing @popperjs/core/lib/popper-lite as described in the documentation here. Since this is intended as a client-side UI library, it would be great to not rely on Node.js variables.

[atomiks]

This is intentional – it's how we can distinguish between DEV and PROD for your application so we can add warning messages without bloating the production bundle size. Every other modern lib uses the same technique.

If you're using Rollup, you need to use rollup-plugin-replace, set environment variables and replace process.env.NODE_ENV with either "development" or "production". webpack makes you choose mode which handles this automatically. Parcel works automatically as well. Otherwise, you can polyfill it globally in the browser if all else fails.

[benjamind]

This is an interesting conundrum for the 'buildless' approach to using es modules in development.

With the advent of web components and es modules, along with es-dev-server to serve them without a build process, it is quite easy for a developer to load a module which uses process.env.NODE_ENV without it having passed through rollup or parcel or any other bundler.

Do you think it might be wise to have these statements fail-safe to production mode if process || process.env is not defined?

For those using es-dev-server there is a workaround today using the Babel module transform-node-env-inline. But there are other es module servers and middlewares out there that similarly serve modules as-is.

For reference this issue also came up in the redux project reduxjs/redux#2907 where I think it has been addressed through removal in esm output, and more recently in graphql too graphql/graphql-js#1819

[FezVrasta]

I think replacing the statements with process?.env?.NODE_ENV !== 'production' would be a good way to handle this issue.

[atomiks]

Isn't the optional chaining operator is super new though?

Redux still has process checks for its "module" field file: https://unpkg.com/browse/redux@4.0.5/es/ but they supply an .mjs version - is that the solution 🤔?

[FezVrasta]

Shipping an .mjs file would be an option too.

(the optional chaining operator would get transpiled in compatible code anyways)

[FezVrasta]

@cuberoot may you take a look at #938 please?

[benjamind]

I think that an .mjs extension for use on the web is actually somewhat problematic. While its ok on node packages, on the web its a little weird, and it also causes problems for typescript users see microsoft/TypeScript#27957.

The browser compatible esm build should probably use standard .js extension, but just not use process.env in the module output at all, or use it in a safe way (check for existence before access).

[FezVrasta]

It’s already reverted to .js on master.

Out of curiosity, how do you plan to use TS and the browser-ready version of Popper together?

I would expect a TS user to import the /lib version with process.env checks, since you need to compile the code to deploy it.

[benjamind]

Oh excellent thankyou!

In spectrum-web-components we're developing with TS. We run a watch process which compiles our tsc code, and this is then served via storybook, via es-dev-server which loads es modules in the browser direct from node_module resolution.

[robrez]

Hitting this issue as well, similar setup as benjamind.

I also hit this in the past w/ redux before it was fixed. Before that PR was merged, a common workaround was monkey patching window.process.env.. eg pwastarter kit

window.process = { env: { NODE_ENV: 'production' } };

I am also working on a custom element that uses the latest popper v2.. super excited about making that switch. I don't wish to have my esm class leave a side-effect on the window like that, but I think it's my best option at the moment

[atomiks]

@robrez you can use dist/esm instead of lib/ which doesn't contain process.env.NODE_ENV checks but works the same, but honestly I would rarely use it since you don't get helpful dev messages which is kind of the whole point :\

edit: dist/esm still contains process.env.NODE_ENV but shouldn't, cc @FezVrasta (edit by @FezVrasta, 2.0.5 fixed this)

[benjamind]

We have found it hard to use the dist/esm build with the typescript type definitions, since the module import name becomes @popperjs/core/dist/esm this does not match the typescript definitions, so we have to re-export the definitions under these new names.

Rather than do the window modification, we have instead opted to use the babel configuration in our test and dev server environments. Unfortunately since we're building a library to be consumed by others, this does not actually solve the problem for our downstream consumers who will have to do similar babel / webpack fixes or set process.env as defined above. Since our library will ultimately get used by others who will most likely bundle their applications this is probably not a major problem.

Honestly I'm not sure what the best approach is here generally speaking, I've searched around the community for guidance and from what I can tell use of process.env in browser specific libraries seems to be a nodejs-ism that has crept into common browser usage. For the crowd trying to move to 'build-less' approaches, use of process.env is undesirable, but it is a very common pattern for defining debug builds and has proliferated in frontend libraries despite requiring a pre-processor step to use it correctly as intended (i.e. dead code blocks that are stripped by bundlers and minifiers).

[FezVrasta]

Would adding a dist/esm/index.d.ts file with:

export * from '../../lib';

enough to add TS definitions to @popperjs/core/dist/esm?

[benjamind]

Doesn't appear to help, also tried other variations with module redeclaration, e.g.:

declare module '@popperjs/core/dist/esm' {
    export * from '@popperjs/core/lib';
}

In all cases we get a type error here:

import {
    popperGenerator,
    defaultModifiers,
} from '@popperjs/core/dist/esm/popper-lite';

The error is:

Could not find a declaration file for module '@popperjs/core/dist/esm/popper-lite'. '[redacted]/node_modules/@popperjs/core/dist/esm/popper-lite.js' implicitly has an 'any' type.
  Try `npm install @types/popperjs__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@popperjs/core/dist/esm/popper-lite';`ts(7016)

The only solution I have found is to mirror all the .d.ts files in the lib folder into the esm folder, for example dist/esm/popper-lite.d.ts:

export * from '../../lib/popper-lite';

This appears to then make the types work properly. This way we have correct types for all the various module imports when doing the suggested treeshaking imports approach.

[FezVrasta]

Have you tried this?

declare module '@popperjs/core/dist/esm/popper-lite' {
    export * from '@popperjs/core/lib';
}

[benjamind]

That would only work if it was in a popper-lite.d.ts and even then the export of popper-lite is not actually the same as core/lib.

I believe the way that typescript resolves imported module types is by first resolving the package and then looking for a file with a .d.ts extension added, e.g. @popperjs/core/dist/esm/popper-lite will first look in <resolved module path>.d.ts which in this case is node_modules/@popperjs/core/dist/esm/popper-lite.d.ts. Next it looks for a typings or types field in the package.json, then a main field (which currently resolves to the CJS build so the module names do not match), and then finally it walks up the resolved path trying to resolve at other node_modules locations it finds.

So I think since you've got no types/typings definitions, and the main field resolves to a non-matching module name from the CJS output, we're a bit out of luck for any out of the box way of doing this without explicitly including the .d.ts for each file in the dist/esm build output.

I think overall it might just be better to have the process.env checks fail safe (i.e. use optional chaining / conditional checks and assume production unless present). You'll protect more users in a wider variety of scenarios that way (bundling / non-bundling) and you can avoid having to have the separate esm build entirely.

[atomiks]

I think overall it might just be better to have the process.env checks fail safe (i.e. use optional chaining / conditional checks and assume production unless present). You'll protect more users in a wider variety of scenarios that way (bundling / non-bundling) and you can avoid having to have the separate esm build entirely.

There are two problems with that:

• DCE won't work with optional chaining syntax (I think) which is required
• Optional chaining syntax is way too new and will break many people's builds, especially if they aren't compiling node_modules (many aren't)

[benjamind]

Did a quick investigation into DCE with uglify-js (i'm assuming terser is somewhat similar)...

With the following file:

if (process && process.env && process.env.NODE_ENV === "debug") {
  console.log("foo");
}

Running uglifyjs test.js -d process.env.NODE_ENV='"production"' -mc this is evaluated to:

process&&process.env,0;

Which seems reasonable, you can further reduce this output with uglifyjs test.js -d process=false -mc:

0;

Agreed though that the output from babel for optional chaining syntax does not work with DCE where this:

if (process?.env?.NODE_ENV === "debug") {
  console.log("bar");
}

Transpiles to:

var _process, _process$env;

if (((_process = process) === null || _process === void 0 ? void 0 : (_process$env = _process.env) === null || _process$env === void 0 ? void 0 : _process$env.NODE_ENV) === "debug") {
  console.log("bar");
}

And minifies to:

var _process,_process$env;"debug"===(null===(_process=!1)||void 0===_process||null===(_process$env=_process.env)||void 0===_process$env?void 0:_process$env.NODE_ENV)&&console.log("bar");

Conclusion?

¯\(ツ)/¯

I honestly don't think there's a 'good' result here for 'buildless' consumers. In those cases you do really start to get into a situation where you can't have any nodejs global variables in your output source. But common practice for debug strings is clearly still falling into the nodejs pattern here.

[gusnaughton]

Hello friends!

If anyone is running into this issue, a workaround solution is injecting <script>var process = {env: {}};</script> into your component. If your application depends on that variable this could have unintended side effects.

Hope this helps!

[FezVrasta]

@gusnaughton you don't need to do that, popper exports a version without the process.env reference.

[gusnaughton]

@gusnaughton you don't need to do that, popper exports a version without the process.env reference.

Our use case requires support for IE11 which doesn't support ES Modules without a polyfill. We haven't yet made a decision on whether to include one for this purpose or not, but I agree, this feels unnecessary.

[FezVrasta]

If you need to support older browsers then you want to use the UMD build, which doesn't include any process.env reference.

[gusnaughton]

If you need to support older browsers then you want to use the UMD build, which doesn't include any process.env reference.

Thank you for this! I will revisit this soon.