testdouble / quibble

Makes it easy to replace require'd dependencies.
93 stars 26 forks source link

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

Closed albertogasparin closed 6 years ago

albertogasparin commented 6 years ago

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 commented 6 years ago

Reproducible with quibble@0.5.3 + 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 commented 6 years ago

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 commented 6 years ago

I am 90% sure this is a regression introduced by https://github.com/testdouble/quibble/pull/18

Chewing on this now.

searls commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Opened https://github.com/browserify/resolve/issues/147

albertogasparin commented 6 years ago

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 commented 6 years ago

Heads up that I've submitted a fix to browserify/resolve as of today. Let's see where that goes https://github.com/browserify/resolve/pull/148

searls commented 6 years ago

This is finally fixed in quibble@0.5.5 -- thanks for being patient!