Make the process polyfill optional

[pkaminski]

Currently, Snowpack will include a (basic) polyfill for Node's process module if it detects any reference to a global process identifier. However:

1. Many modules only reference process.env so they don't need a full polyfill, and for references to process.env.NODE_ENV would actually benefit from having the value inlined (for optimization).
2. Some modules explicitly check for the presence of process to decide whether to run Node-specific or browser-specific code, and having a polyfill will confuse them into a sub-optimal path at best and outright break them at worst.

I still think that the polyfill is a reasonable default but I believe we should be able to turn it off. I'd like to propose a new installOptions.polyfillProcess option with allowed values "basic" (the current polyfill), "env" (inlining of values from installOptions.env only), and "none" (ignore any references to process).

What do you think?

[FredKSchott]

For context: our original process.env polyfill did do a simple find-replace on certain values, but we got a ton of bugs reported where find-replace just didn't cut it and ended up breaking code that would otherwise work. For that reason, we moved to a more powerful process object polyfill.

Now that we have that working, I actually agree that a much simpler process.env.XXXXXX find-replace could be useful to add back for NODE_ENV + all configured env variables, so that Rollup could better optimize and remove dead-code that is gated by some process.env check.

For (2), can you share an example of a module checking for process to decide if it's Node or Browser specific? Since most frontend bundlers do polyfill process in some way, I actually don't believe that this would work in the wild. Usually, they check for something like process.platform === 'browser' or process.browser === true, both of which our polyfill handles.

If we could add back the simple process.env.XXXXXX find-replace as a default for everyone, then I don't think we'd need a new option. What do you think?

[seriousManual]

Hi,
I just ran into the same issue regarding the polyfilling of process:
One of my modules (https://www.npmjs.com/package/hirestime) is doing "feature detection" to determine if it's running in node or in the browser.
Since process is always thinks it runs in node and therefore executes the node path:
https://github.com/seriousManual/hirestime/blob/master/src/index.ts#L48

I tried the configuration switch installOptions.polyfillNode but that does not seem to work.

[FredKSchott]

polyfilling process in the browser is fairly common. As far as I know webpack and parcel do this by default, and our polyfill is a common Rollup one.

Is there a better way to check if you're in the browser? process.browser or process.platform === 'browser', maybe?

[seriousManual]

Does snowpack respect the "browser" field from the package.json? (https://docs.npmjs.com/cli/v6/configuring-npm/package-json#browser)
In that case it would probably the best idea to use that to export a browser only implementation which could look something like this:

import {browserSpecificImpl} from './my/code';

export default browserSpecificImpl

If I understand that correctly, the process polyfill will be inserted anyways, though in a build process it will be stripped out by treeshaking/dead code elemination, is that assumption correct?

[FredKSchott]

we support the browser field!

[seriousManual]

thanks!

[pkaminski]

Yep, I saw the simple find-replace in the code history. I was actually thinking of doing something a bit more sophisticated by walking the AST like the current @rollup/plugin-inject does, so it shouldn't have any more false positives than the current method.

And yeah, now that I think about it more I guess the modules I've seen check for module or require, not process. One remaining concern is the additional size of the process polyfill when it's not actually needed -- will Rollup tree shake most of it out if, e.g., a module only references process.env[somethingDynamic]? If so, I agree we don't need a new option. (And if a module has only literal env references, then if we run the process.env.FOO transform first the polyfill injector shouldn't trigger anyway.)

[FredKSchott]

I believe Rollup tree-shakes most of it, although we should confirm that.

So to summarize, I'm +1 on a process.env.XXXXX pre-pass simple find-replace. But, I am -1 on adding anything else additional at this point. The benefits are too slight / would impact too few users vs. the added complexity and chance to break users.

[pkaminski]

When you say "simple find-replace" do you literally mean a regex-based replace, or a more precise AST-based one? If the former, don't you think this might trigger the same bugs as the original process.env "polyfill" did? If the latter, would you be open to merging the process.env and process polyfill plugins so we only need to parse and walk the AST once?

[FredKSchott]

Yup, regex-based. The bugs that saw came from trying to do too much with the regex find-replace, like replace the entire process.env or process. with ({}). which broke different packages.

Luckily, regex find-replacing process.env.XXXXXX is so common in the bundler space that all packages I've ever encountered are designed to work with it, and many are actually optimized for it since replacing the object reference with an actual string is the only way to get Rollup to actually eliminate the dead code kept inside of some if (process.env.NODE_NEV === 'production') code block.