node-util icon indicating copy to clipboard operation
node-util copied to clipboard

circular reference cause `inherits()` became empty object

Open normanzb opened this issue 1 year ago • 5 comments

the line below refs to inherits() from inherits module, but inherits module reference back to util module by checking the implementation on util module first, causing the first require('inherits') to returns empty object (default exports is empty object), if then util.inherits() gets call immediately it will throw.

problematic line in question: https://github.com/browserify/node-util/blame/ef984721db7150f651800e051de4314c9517d42c/util.js#L592

the line needs to be updated to require('inherits/inherits_browser')

normanzb avatar Jul 12 '23 13:07 normanzb

I don't see why - the inherits package has a "browser" field which bundlers should be handling automatically, and in node it would need to be "inherits" directly.

ljharb avatar Jul 12 '23 13:07 ljharb

@ljharb wow that's quick reply, we are using it in react native, neither node nor browser, there are potentially more js runtime out there...

normanzb avatar Jul 12 '23 13:07 normanzb

Sure, that's fine - metro should either be able to handle circular references or it should use the browser endpoint, since RN is much more browser-like than node-like.

This is a node module, so it's up to transformation tools to handle using it in anything other than node (altho "browsers" are especially important).

ljharb avatar Jul 12 '23 13:07 ljharb

thx for the reply, 2 cents of my understanding:

This is a node module

this is a module that mimic a node module, not the node module itself. the true util is a built in nodejs module, it is unlikely anyone would purposefully install this polyfill package in a node env -- if they do most likely is because the lib was included as a part of a dep. most likely the reason this lib is running is because it is not a node env hence no built-in util. based on that context having another check of require('util') seems redundant to me, it either ref back to this lib or this lib competely no need to be ran at all.

it should use the browser endpoint

just fyi, react native is far from the browser, it doesn't have dom api, it doesn't have the globals available in browser and there is no regulation and authorities that control what is available in react nataive.

so unfortunately there is no golden rule or silver bullet in terms of which platform to use, package support is difficult to find, but RN is a growing community, i wish the maintainer of this lib do consider support of it in the future.

normanzb avatar Jul 12 '23 14:07 normanzb

ha, true enough :-) this is a node module intended for transparent replacement by a bundler for browser usage.

Either way, supporting circular references are required to support CJS as well as ESM, so that seems like the underlying problem here for metro.

ljharb avatar Jul 12 '23 14:07 ljharb