Skip to content

Quibble messes up require core modules/dependencies with same name #22

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

Closed
albertogasparin opened this issue Jan 9, 2018 · 9 comments
Closed
Labels

Comments

@albertogasparin
Copy link

Looks like quibble.absolutify() is not careful enough to correctly handle core modules that happen to have the same name as installed dependencies.
For instance, tests of my suite broke because I'm using util core module and one of my dependencies loads util polyfill in node_modules (via require('util/')) beforehand.
Quibble fakeLoad() is first called with "util/", [Module], false and it correctly
resolves it to /node_modules/util/util.js. Then fakeLoad() gets called with "util", [Module], false but instead of recognising it as a core module, it resolves it to /node_modules/util/util.js again.
As I'm using util.promisify (not available in the polyfill) my tests break as my code ends up consuming the poyfill instead of the node core module.

@albertogasparin albertogasparin changed the title Quibble messes up require core/modules with same name Quibble messes up require core modules/dependencies with same name Jan 9, 2018
@albertogasparin
Copy link
Author

albertogasparin commented Jan 9, 2018

Reproducible with [email protected] + node 8.9.1
Same code with v0.5.1 works fine, as quibble.absolutify('util', '/path/to/file') correctly returns util regardless of the presence of node_modules/util

@searls
Copy link
Member

searls commented Jan 9, 2018

Thanks for this report. Any chance you could try adding a failing test or example to the build? I'm not likely to have time this week to get to this issue

@searls searls added the bug label Jan 9, 2018
@searls
Copy link
Member

searls commented Jan 9, 2018

I am 90% sure this is a regression introduced by #18

Chewing on this now.

@searls
Copy link
Member

searls commented Jan 9, 2018

I'm having a really hard isolating this into a test case in the repo, because for whatever reason I can't reproduce it when quibble is a symlinked dependency, only when it's actually installed in node_modules (despite master and v0.5.3 being the same thing).

@searls
Copy link
Member

searls commented Jan 9, 2018

I will say one thing, which is that this appears to be a bug in the node-resolve package. Given a similar setup to what you described, notice that it works fine for util/ but differs from Node's require.resolve function for util:

> require('resolve').sync('util')
'/Users/justin/code/testdouble/quibble/regression-cases/core-shadowing/node_modules/util/util.js'
> require('resolve').sync('util/')
'/Users/justin/code/testdouble/quibble/regression-cases/core-shadowing/node_modules/util/util.js'
> require.resolve('util')
'util'
> require.resolve('util/')
'/Users/justin/code/testdouble/quibble/regression-cases/core-shadowing/node_modules/util/util.js'

@searls
Copy link
Member

searls commented Jan 9, 2018

Opened browserify/resolve#147

@albertogasparin
Copy link
Author

As this is not moving on browserify, could you revert the changes, and eventually re-release them as a new minor version of quibble?
We cannot update our projects as npm tries to get 0.5.3 for any testdouble v3.x 😢

@searls
Copy link
Member

searls commented Mar 23, 2018

Heads up that I've submitted a fix to browserify/resolve as of today. Let's see where that goes browserify/resolve#148

@searls
Copy link
Member

searls commented Apr 27, 2018

This is finally fixed in [email protected] -- thanks for being patient!

@searls searls closed this as completed Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants