Make behavior consistent with require.resolve() for "shadowed" core modules

[searls]

Resolves #147

• Add a failing test
• @ljharb confirms the test looks OK
• Fix the issue
• Deal with whatever breaks.

Hows this look for a start?

[ljharb]

Test looks great (along with the appropriate fixtures).

[searls]

I'm a bit stumped on how to proceed. Internally to Node's require and require.resolve functions is this check which invokes NativeModule.nonInternalExists, which isn't (AFAICT) publicly available.

As a result, short of shipping a white list of known-to-be-internal module names, I'm unsure of any solution that wouldn't reach into Node internals, which (I presume) would defeat the purpose of this module for use in browserify'd code.

Is there anything in browserify to detect and intercept native modules, or is it actually relying on this behavior (of preferring shadowed modules over internals) in order to accomplish that?

[ljharb]

We already have a whitelist of known-to-be-core module names; what's an example of an "internal" module name you'd want to use?

[searls]

Well, the example I've been using all along is "util", which I'm sure you have covered. I'd also expect not-publicly-documented-but-publicly-accessible modules like 'module' would be covered by it.

Where is this list so this condition can use it?

[ljharb]

https://github.com/browserify/resolve/blob/master/lib/core.json, via the https://github.com/browserify/resolve/blob/master/lib/core.js object.

[searls]

I got this new test passing, but identified what I think will be a problem in that this new behavior conflicts with #25. It's my opinion that #25 was essentially incomplete/incorrect, because even as far back as 0.10.48, require.resolve behaved the way specified in this PR's test (that is to say, 'punycode' would always return the core module wherewas 'punycode/' would return the shadowed one).

Since it's duplicative, I would recommend deleting the tests @michaelficarra added in #25. (which I did here: d00883c )

This does raise a decision point, however, which is that surely there are people who are depending on this behavior. Perhaps they want to shadow, maybe even transitively, a native module with an npm package and don't want to or cannot change the string passed to require() to append a '/' character.

Thoughts?

[ljharb]

Given the comments on that issue, I think you're right - let's make sure we're testing the proper thing, and if this is a breaking change, so be it (there's others I can make at the same time).

If someone's doing require('punycode') and relying on it resolving to node_modules/punycode rather than the core module, then I think they need to add the slash. resolve already exports isCore, so they have the tools to work around it.

[ljharb]

Looks good - since this is breaking, I'm going to let it sit a bit before merging.

[searls]

Thanks @ljharb -- how will you know that it's sat long enough? I've got a few users waiting on this to land, because it's causing an upstream bug in testdouble.js

[ljharb]

I'm traveling atm, but after I'm back in town, I plan to try to merge any outstanding non-breaking items, and cut a release, and then i'll take another look at this one.

[searls]

Cool, let me know if I can help

[searls]

Yay! As soon as you cut a release, I can fix the downstream issues my users were having

[searls]

Looks like this landed in 1.7.1 -- Thanks @ljharb!

[ljharb]

This has not yet actually landed in v1; I'll try to backport it.

[ljharb]

Done in 4c9b260; it'll be in the next v1 release.