Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't assume process.env exists in the global scope. #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MartinSherburn
Copy link

Some platforms may not have process.env in the global scope and this shouldn't prevent util from running. Currently it will thrown an exception on import if process.env doesn't exist.

…script to fail on platforms that don't support it.
@ljharb
Copy link
Member

ljharb commented Jun 4, 2021

This is a node module; process.env always exists.

A bundler bundling this node module is broken if it doesn't ensure this code works in the target environment. Are you using such a broken bundler?

@MartinSherburn
Copy link
Author

This is a node module; process.env always exists.

Why do other parts of the file do this "typeof process !== 'undefined'" check then? e.g. https://github.com/browserify/node-util/blob/master/util.js#L76 . It seems inconsistent to me.

Are you using such a broken bundler?

No, I'm using node-util an a platform that doesn't have process in the global scope. Other than this minor problem all the other code works fine.

@ljharb
Copy link
Member

ljharb commented Jun 4, 2021

Which platform?

@MartinSherburn
Copy link
Author

Which platform?

Hermes https://hermesengine.dev/ . But does it matter? From the description of node-util, it is designed to work in any environment right?

Node.js's util module for all engines.
This implements the Node.js util module for environments that do not have it, like browsers.

@ljharb
Copy link
Member

ljharb commented Jun 4, 2021

Yes, it does matter. node modules can only run unmodified on node - they often use require, for example - and a build tool is always required for other platforms, whether server or browser.

That the file already has a typeof process check does seem to suggest that this change is reasonable, though.

util.js Outdated Show resolved Hide resolved
Co-authored-by: Jordan Harband <[email protected]>
@JounQin
Copy link

JounQin commented Aug 30, 2021

Any news? This package is used by assert package, which is a built-in node module, but some packages can also be run on browser like micromark use assert inside, so I have to install npm assert on development and strip them on production building.

For now, I have to define a process.env.NODE_DEBUG in vite.config.ts for workaround.

@ljharb
Copy link
Member

ljharb commented Aug 30, 2021

File a bug on vite, if it’s broken the same way webpack 5 is. A node module bundler must be able to transparently provide browser versions of node core module and globals, including process.

@JounQin
Copy link

JounQin commented Aug 31, 2021

File a bug on vite, if it’s broken the same way webpack 5 is. A node module bundler must be able to transparently provide browser versions of node core module and globals, including process.

@ljharb

There is workaround by custom defined process.env. NODE_DEBUG as I described.

I don't think it is a bug of vite, not of node-util neither of course.

But like inspect-js/has-symbols#11 (comment), adding a check here is great.

@ljharb
Copy link
Member

ljharb commented Aug 31, 2021

It's definitely a bug, because it's talking node modules as input and not providing node globals.

In the case of has-symbols, it works identically because it's a language global, available in both node and browsers. In this case, when process is undefined, the precondition for running this entire package has failed to be met, and the entire environment is invalid.

@JounQin
Copy link

JounQin commented Aug 31, 2021

@ljharb

Personally, I don't think polyfilling process or any other node specific features is the design philosophy of vite, just like webpack@5, the end users must polyfill them manually.

@ljharb
Copy link
Member

ljharb commented Aug 31, 2021

@JounQin i'm aware of that, but it being their design philosophy doesn't prevent it from being wrong.

@JounQin
Copy link

JounQin commented Aug 31, 2021

@JounQin i'm aware of that, but it being their design philosophy doesn't prevent it from being wrong.

@ljharb That's why we're here talking about how to support them.

It is said in README:

This implements the Node.js util module for environments that do not have it, like browsers.

If your code runs in the browser, bundlers like browserify or webpack also include the util module.

@ljharb
Copy link
Member

ljharb commented Aug 31, 2021

The best way to support them is to advise in the readme how users can fix these broken bundlers' mistakes, and to link them to the relevant issues to help surface the pain it's causing.

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Aug 31, 2021

That the file already has a typeof process check does seem to suggest that this change is reasonable, though.

The other typeof process check is kind of vestigial, it (used to?) exist in Node.js in case the module runs during startup when process is not yet available, but when this was last copy-pasted other parts of the Node.js code already relied on process being available from the start.

Its tempting to just add the check since I don't think anyone has ever used NODE_DEBUG in the browser, but that would make it a breaking change if an additive feature (in node.js) relies on the process global in the future. So i do agree with ljharb that the correct approach here is to document what users need to do.

IIRC it's something like this:

npm install util process
module.exports = {
  plugins: [new ProvidePlugin({ process: 'process/browser' })],
  resolve: {
    fallback: {
      process: require.resolve('process/browser'),
      util: require.resolve('util/')
    }
  }
}

If there is a webpack plugin that just provides ALL the Node.js builtins that would be ideal…

@wagenet
Copy link

wagenet commented Jan 10, 2023

I ran across this issue in the following way: Our application uses sinon for browser tests. Sinon has a require('util') call but does not actually have util as a dependency and this attempt would fail silently. However, we also installed aws-sdk in the same monorepo for a command line tool. This does have util as a dependency. Now that util was in our tree, sinon started being able to require it. At this point we started seeing failures because process was not defined. I have no interest in adding process to our dependencies since we have no other reason for it. I would be happy to see the process check extended further.

@ljharb
Copy link
Member

ljharb commented Jan 10, 2023

@wagenet that's because util is a core module and should NOT be a dependency (same with process). This package is silently substituted in by bundlers (ones that work properly, at least).

@scarletquasar

This comment was marked as abuse.

@parikshit223933
Copy link

I am not getting it. What is the potential harm in adding a safe check?

@ljharb
Copy link
Member

ljharb commented Jan 23, 2023

@parikshit223933 enabling broken tools to work anyways is harmful for the ecosystem; this also adds extra bytes for every user, despite only users of broken tools needing it.

@parikshit223933
Copy link

Your point is 100% valid. Should we remove typeof process !== 'undefined' on line 76 as well?

@ljharb
Copy link
Member

ljharb commented Jan 23, 2023

Yes, we probably should - I'm not sure why that check is there.

@benmccann
Copy link

This is a node module; process.env always exists.

This is not a Node module, but rather it's a polyfill of a Node module for the browser. Browser code would really be better distributed as ESM instead of CJS. Then it could be used directly without bundling. At that point it'd make a lot of sense not to assume that process.env exists as that's something that exists only in Node and not in the browser

@ljharb
Copy link
Member

ljharb commented May 23, 2023

@benmccann that's its purpose, to be used in a browser, but it has package.json - it's a node module. It's meant to work with bundlers, the standard for which is browserify, which invented the concept. A bundler that deviates from this defacto standard is broken, and it would be harmful to make changes that enable people to use broken bundlers.

As for "without building", that's just not feasible in modern web dev, and native ESM is both slower and less fully-featured than CJS, and, since ESM can't be consumed by CJS, but CJS can be consumed by ESM, CJS is the more universal format (assuming a build process exists).

@benmccann
Copy link

It's a bit rich to me that you would pick the least used of all bundlers and call it the standard. Just because something's first, doesn't mean that's what the standard is. Today Chrome, Firefox, and Safari set the standards for web browsers. No one goes around screaming that they're breaking standards because they're deviating from the behavior of Nexus and Mosaic. If my memory serves me correctly esbuild, Vite, and Rollup do not polyfill process.env while webpack and browserify do. So I believe the majority of users are using bundlers which do not polyfill process.env. And similarly, while CJS was first, the standard is ESM.

https://npmcharts.com/compare/browserify,vite,webpack,rollup,esbuild

@ljharb
Copy link
Member

ljharb commented May 24, 2023

Webpack 5 no longer does, I believe, which is why it's a huge problem for both users and maintainers (https://blog.sindresorhus.com/webpack-5-headache-b6ac24973bf1 unfortunately no longer loads), and why, I suspect, webpack 4 on npm has 7m downloads a week and webpack 5 has merely 1.6m.

esbuild, vite, and rollup making this choice also causes no end of filed issues on projects, and the solution for all of them is "sadly, your bundler forces you to configure it properly instead of doing it by default".

CJS is a standard, ESM is not "the" standard - it's just one of multiple.

@benmccann
Copy link

webpack 5 actually has more usage than webpack 4. It's just that webpack 5's usage is split amongst several versions while webpack 4's usage is concentrated in the terminal version. E.g. these half a dozen version have over 9m downloads between them:

5.83.1	2,135,676
5.82.1	1,186,589
5.76.1	1,152,487
5.75.0	2,361,856
5.74.0	1,655,382
5.73.0	689,447

At this point, I'm not really sure what the argument is for continuing to use process.env. You can argue that bundlers should polyfill it, but the fact is that none of the major bundlers do that and they're moving in the opposite direction. At this point, continuing to use process.env is not practical and would just be causing everyone pain to fight a losing ideological battle.

@ljharb
Copy link
Member

ljharb commented May 24, 2023

Thanks, fair point on webpack 5 usage - it's still barely more than half, after being out for 3 years.

This is a node module. process.env always exists. If none of the major bundlers are doing the right thing, then they should be persuaded to do so - pressuring individual maintainers to cave, one package at a time, isn't an ethical or sustainable or practical approach.

@arinthros

This comment was marked as abuse.

@scarletquasar

This comment was marked as off-topic.

@ljharb

This comment was marked as resolved.

@jerry2013
Copy link

I really don't get the part "This is a node module. process.env always exists." when the pack itself specifically says it's for things like "browsers".

Logically,

This implements the Node.js util module for environments that do not have it, like browsers.

plus

If your code runs in Node.js, util is built in.

means this package is for non-node environments, which means global.process is not implied.

@ljharb
Copy link
Member

ljharb commented Jan 4, 2024

@jerry2013 sure. But a node module bundler is required, and a properly working one polyfills node globals.

@JounQin
Copy link

JounQin commented Jan 4, 2024

But a node module bundler is required, and a properly working one polyfills node globals.

Node module bundlers have already changed how they work. Node.js is not the only special js runtime anymore.

@benmccann
Copy link

At this point, the vast majority of users have migrated to webpack 5, so it's an ever shrinking minority of bundlers that polyfill process.env.

Screenshot from 2024-01-03 20-23-58

@jerry2013
Copy link

I get that since the code uses process in several places, e.g. process.nextTick(), plus some deprecation and debug related things, simply shim the process.env wouldn't be enough to have everything "work".

On the other hand, given this package is intended for "non-node" environment, would it be sensible to just remove the "deprecation and debug thing", then replace process.nextTick by a using promise?

@jerry2013
Copy link

well, removing "deprecate" wholesale would not work. but I did find why the code has the check for process in there (#11)

@ljharb
Copy link
Member

ljharb commented Jan 4, 2024

@JounQin it was never about it being the "only" one, it's the dominant one and will remain so for the foreseeable future. If you want to use the majority of things that are on npm, then they will be designed for and tested in node (which this package is, ofc), and as such, you need a working node module bundler to use them in a browser, and that will always be the case.

@benmccann my experience is that many who have upgraded have done so by fixing the unfortunate decision to not polyfill things by default.

@JounQin
Copy link

JounQin commented Jan 4, 2024

@ljharb Why are you forcing the new bundlers to compliant with your such only one module? Why a simple typeof process !== 'undefined' can not be added to make the whole world happy without breaking anyone?

@ljharb
Copy link
Member

ljharb commented Jan 4, 2024

@JounQin there are many thousands of modules on npm that assume the existence of node globals and core modules, and many of these will never be updated. The existing corpus of npm is what "forces" this form of backwards compatibility, and it's not helping anyone to go complain on individual packages in the hopes that they get you closer to an impossible goal.

The only way to make the "whole world happy" is for bundlers to go back to the eternally correct behavior of automatically polyfilling needed things.

@JounQin

This comment was marked as off-topic.

@JounQin

This comment was marked as spam.

@browserify browserify locked and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants