[BUG] Webpack builds are failing as new release 7.5.0 requiers node.js util

[janhommes]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

A webpack build is failing with the following error:

./node_modules/xxx/node_modules/semver/classes/semver.js:30:46-69 - Error: Module not found: Error: Can't resolve 'util' in '/runner/_work/xxx/node_modules/xxx/node_modules/semver/classes'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
	- add a fallback 'resolve.fallback: { "util": require.resolve("util/") }'
	- install 'util'
If you don't want to include a polyfill, you can use an empty module like this:
	resolve.fallback: { "util": false }

It was working in version 7.4.0

Expected Behavior

The webpack build should work without adding any node.js dependency as we want to use semver in the browser.

Steps To Reproduce

1. Setup a webpack build
2. add semver
3. Run webpack
4. See error above

I assume it was introduced due to this change:
v7.4.0...v7.5.0#diff-7917f8ef42ff71df5bde71c259d15feab8b147e3e6c2fde11b56cbf42cadea16R19

Environment

• npm: 6.14.17
• Node: 14.9.3
• OS: Ubuntu 20.04
• platform: Linux

[wraithgar]

This is obviously one of those "soft" areas of semver. We have an engines declaration and test in all the versions laid out there. Other environments are technically outside the support matrix.

Given the presence of tools like https://github.com/browserify/node-util specifically for things like this I wonder if fixing one's webpack setup is appropriate.

[janhommes]

This is obviously one of those "soft" areas of semver. We have an engines declaration and test in all the versions laid out there. Other environments are technically outside the support matrix.

Given the presence of tools like https://github.com/browserify/node-util specifically for things like this I wonder if fixing one's webpack setup is appropriate.

You might be right if you say node-semver does not support web apps. On the other hand, webpack is quite a common bundler, and a lot of people seem to use semver on the web(as you can see, in less than one day, already 4 other people seem to have run into this issue). At the same time, the breaking change is only a developer experience improvement, nothing really of high importance. Maybe there is even an easy workaround with JSON.stringify to fulfill both goals (not breaking, and logging the correct error)? Not sure, not a node expert.

[wraithgar]

You might be right if you say node-semver does not support web apps

I might be wrong too, this is a good issue that got opened and I'd rather have a short discussion instead of acting in haste.

If we decide to fix this, it would be good also to decide there is a basic support level in webpack et al for this lib and somehow enforce it in CI. I don't know what that looks like though.

Maybe there is even an easy workaround with JSON.stringify

This is definitely the right fix if we want to remove util.

[ljharb]

This is just webpack 5 being broken - a working node module bundler automatically polyfills node core modules whenever possible. I don't think it's part of semver to maintain compatibility with broken tools.

[wraithgar]

> JSON.stringify(y)
Uncaught TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Object'
    |     property 'x' -> object with constructor 'Object'
    --- property 'y' closes the circle
    at JSON.stringify (<anonymous>)
> util.inspect(y)
'<ref *1> { x: { a: 1, y: [Circular *1] } }'

JSON.stringify is not a good replacement for util.inspect

[ljharb]

@wraithgar you could certainly use https://npmjs.com/object-inspect if you like :-)

[wraithgar]

It does look like webpack itself has made the decision to make polyfills the responsibility of the consumer, and when they're encountered it's up to them to add where needed.

Bringing in a net new dependency for this error message seems like the wrong choice. I think this is down to "how webpack works" and "we can't add internals that need polyfilling" is not a contract node utils should need to support.

[ljharb]

I completely agree.

[wraithgar]

https://github.com/inspect-js/object-inspect/blob/main/util.inspect.js ?

[ljharb]

@wraithgar that's skipped in bundlers via https://github.com/inspect-js/object-inspect/blob/main/package.json#L79-L81 - the sole purpose is to get at node's custom util.inspect Symbol when available.

[wartab]

So this is essentially meaning we need another dependency to be bundled with our applications to show an error message for when someone happens to feed complete garbage to semver? Most people don't even really know how to configure Webpack to do that when Webpack just happens to be what their application is built with (such as Angular).

That seems unreasonable and quite useless.

On top of that, that modification was completely out of scope of that pull request.

[wraithgar]

So this is essentially meaning we need another dependency to be bundled with our applications

This appears to be the way webpack works, yes.

[wraithgar]

The modification was in scope because it was throw on bad version with correct error message. Correcting the error message was part of the fix

[wartab]

My bad on the second comment, I was reading the wrong part of the version change. It's still unreasonable.

[tjenkinson]

IMO if previously this library used to work in a browser without any node dependencies, the benefit of the slightly clearer error message is not worth breaking that, and maybe supporting browsers is something this package could officially support?

[ljharb]

@tjenkinson it's webpack 5 that doesn't support browsers, since "supporting browsers" assumes automatic polyfilling of core modules and globals, which is what bundlers have been doing for over a decade.

[tjenkinson]

Hmm I have a different understanding then. if I installed a package that was meant to support browsers I wouldn't expect it to reference any node built-in dependencies

[ljharb]

That's not how the ecosystem has treated it for over a decade - packages are node modules, and a node module bundler allows them to work in browsers, and the "browser" field is how node-specific functionality can be overridden by a package to provide browser-specific functionality as needed.

[silverwind]

Depending on Node for something that can run in all JS environments is a no-go, and this module strikes me as something that at least originally was meant to be universally compatible.

[ljharb]

node is the universe here; it’s npm.

[SkReD]

I expect that such essential packages are tried to be as self contained as possible. Npm ecosystem already suffers from problem of huge dependency trees

[okdistribute]

I find it ironic that a package called 'semver' has introduced a breaking change in a minor release.

[ljharb]

@okdistribute to be clear, it’s not a breaking change for its target envs - bundlers aren’t that - but also, it works fine with non-broken bundlers.

[okdistribute]

It broke my build on an npm install with a previously working package lock. I now have to pin semver to 7.4.0 so it will not get updated. I understand it wasn't your intention to break the package, but the impact is that 7.5.0 is breaking builds without warning

[rodperottoni]

@okdistribute to be clear, it’s not a breaking change for its target envs - bundlers aren’t that - but also, it works fine with non-broken bundlers.

Webpack is a broken bundler?

Right... 😂

[ljharb]

@rodperottoni yes, as of v5, it is a broken bundler: https://web.archive.org/web/20201011110014/https://blog.sindresorhus.com/webpack-5-headache-b6ac24973bf1?gi=8ecd10aab21d

[rodperottoni]

So what defines a bundler as broken / functional is its ability to polyfill NodeJS APIs? That just sounds like your opinion, and I wholeheartedly disagree with it. It's a shame newer versions of semver cant be used by projects using Webpack or Metro anymore.

[wraithgar]

I'm removing the "Needs Discussion" flag from this issue, the ongoing comments do not feel productive. Can we please keep the comments on topic, namely:

If we want to revert this, it needs to be done in a way that also sets up some sort of linting or other testing so that "does not use x internals from node" is part of the contract, which it wasn't before.

[DomiR]

Hi @wraithgar, I wanted to share my thoughts on this. Personally, I think it would be best to revert the changes. As it stands, there doesn't seem to be any other spec compliant semver package available on npm, at least not one that shows up when searching for "semver". I strongly believe that semver should remain a library that compares version strings using "ECMAScript compliant JS". If this lib would be using node libraries to make these comparisons more performant it would be a different story, but the marginal dx gain from using node's util inspect for better error output doesn't seem worth it.

Given that this repo is called "node-semver", I can certainly understand if it were to move in this direction but then we should consider renaming the npm package to "node-semver" as well. Otherwise it's possible that we'll see a fork emerge soon that removes that one line. I think this package is quite popular in non-node environments like React Native, as well as many other "update handling" clients (SPAs). It would be a shame if the efforts of maintaining a version comparing library were now split up.

[tjenkinson]

If we want to revert this, it needs to be done in a way that also sets up some sort of linting or other testing so that "does not use x internals from node" is part of the contract, which it wasn't before.

Opened a PR here that does that if we go with that approach: #559

[wraithgar]

Thank you @tjenkinson this is what was needed.