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

When core modules are shadowed, behavior differs from require.resolve #147

Closed
searls opened this issue Jan 9, 2018 · 4 comments · Fixed by #148
Closed

When core modules are shadowed, behavior differs from require.resolve #147

searls opened this issue Jan 9, 2018 · 4 comments · Fixed by #148

Comments

@searls
Copy link
Contributor

searls commented Jan 9, 2018

@albertogasparin opened testdouble/quibble#22 which led me to this realization: if a core module (say, util) is shadowed by an npm package of the same name (say, util), then the behavior of this module will differ from the behavior of require.resolve in Node-core.

If attempting to load the shadowed package (as on would with require('util/')), the two functions are in agreement:

> require.resolve('util/')
'/Users/……/node_modules/util/util.js'
> resolve.sync('util/')
'/Users/……/node_modules/util/util.js'

But subsequent attempts to resolve the core module with plain ol' 'util' will differ:

> require.resolve('util')
'util'
> resolve.sync('util')
'/Users/……/node_modules/util/util.js'

(To pre-empt any request I simply use require.resolve(), I'm currently using this package for its preserveSymlinks option which Node's function lacks and which I've proposed be added here)

Eager to hear your thoughts on this and whether you'd consider this behavior to be a bug and advise how we should proceed or could help

@searls searls changed the title When core modules are shadow, behavior differs from require.resolve When core modules are shadowed, behavior differs from require.resolve Jan 9, 2018
@ljharb
Copy link
Member

ljharb commented Jan 9, 2018

I believe this might be related to nodejs/node#15015

I also suspect this is how browserify resolves some of its native module shims, such that changing this behavior might break it.

What’s your use case for ever shadowing core modules?

@searls
Copy link
Contributor Author

searls commented Jan 9, 2018

Understood. I'm not very familiar with browserify internals at all, so I'm curious if you can see any path forward in reconciling this.

What’s your use case for ever shadowing core modules?

Who knows man, I'm just a middle-man lib maintainer trying to identify the root cause of the aforementioned bug report ( testdouble/quibble#22 ), which mentioned that the reliance on the util package was brought in via a transitive dependency and not their direct fault.

@albertogasparin
Copy link

I found the bug while doing unit tests. I'm using mocha, which requires assert which is dependent on util/(don't know why, probably for browser compat?).
My code under tests requires node core util, however when quibble intercepts the call (calling resolve.sync('util')) the result is the partial implementation instead of the real core module.
I wouldn't have notice if not because the method I'm using (promisify) has just been added to node core util and it is still missing on util/.

@ljharb
Copy link
Member

ljharb commented Jan 10, 2018

It'd be useful to prepare a PR that "fixes" this behavior, so we can take a look at the tests.

I agree that resolve.sync and require.resolve should ideally function as similarly as possible.

searls added a commit to searls/resolve that referenced this issue Mar 23, 2018
Given 'util' as an example native module which is also installed as an
npm package, then in order for browserify/resolve to act consistently
with native `require.resolve`, these should be true:

```
resolve.sync('util') // returns 'util'
resolve.sync('util/') // returns 'node_modules/util/index.js'
```
ljharb pushed a commit to searls/resolve that referenced this issue Apr 5, 2018
Given 'util' as an example native module which is also installed as an
npm package, then in order for browserify/resolve to act consistently
with native `require.resolve`, these should be true:

```
resolve.sync('util') // returns 'util'
resolve.sync('util/') // returns 'node_modules/util/index.js'
```
ljharb pushed a commit to searls/resolve that referenced this issue Apr 5, 2018
Given 'util' as an example native module which is also installed as an
npm package, then in order for browserify/resolve to act consistently
with native `require.resolve`, these should be true:

```
resolve.sync('util') // returns 'util'
resolve.sync('util/') // returns 'node_modules/util/index.js'
```
ljharb pushed a commit to searls/resolve that referenced this issue Apr 5, 2018
Given 'util' as an example native module which is also installed as an
npm package, then in order for browserify/resolve to act consistently
with native `require.resolve`, these should be true:

```
resolve.sync('util') // returns 'util'
resolve.sync('util/') // returns 'node_modules/util/index.js'
```
ljharb pushed a commit to searls/resolve that referenced this issue Apr 7, 2018
Given 'util' as an example native module which is also installed as an
npm package, then in order for browserify/resolve to act consistently
with native `require.resolve`, these should be true:

```
resolve.sync('util') // returns 'util'
resolve.sync('util/') // returns 'node_modules/util/index.js'
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants