fixes #25: resolve modules with the same name as node stdlib modules #30

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@michaelficarra

Fixes #25. In particular, this prevents me from resolving https://npmjs.org/package/punycode and https://npmjs.org/package/querystring using node-resolve.

@michaelficarra

Ping @substack. This bug is still getting in my way.

@michaelficarra

Cool, looks like you've just committed it as 9637b60. Closing this, since Github didn't detect is as a merge.

@michaelficarra michaelficarra referenced this pull request in michaelficarra/commonjs-everywhere Nov 26, 2013
Closed

resolve core modules consistently now that substack/node-resolve#30 is merged #87

@substack
Owner

Merged in 0.6.0. The upstream dependencies will be updated shortly.

@michaelficarra

@substack: which upstream dependencies do you mean?

@substack
Owner

Waiting for @defunctzombie to update browser-resolve then I can upgrade module-deps.

@michaelficarra

Oh, I was just trying to get these to work like the others.

@defunctzombie
@substack
Owner

@defunctzombie can you update browser-resolve to use resolve@0.6.0?

@defunctzombie

Updated version of resolve published in browser-resolve v1.2.0

@substack substack referenced this pull request in substack/node-browserify Nov 26, 2013
Closed

How do I specify browser versions of core modules? #356

@eventualbuddha

This seems to have caused this library to diverge from the actual node resolve algorithm, which pretty clearly states:

require(X) from module at path Y
1. If X is a core module,
   a. return the core module
   b. STOP
…

This is the root cause of rollup/rollup#544. It can be worked around, but I believe this should either be reverted or a note should be added to the README that explains in what way this library differs from the real node resolve algorithm.

@boneskull

I don't know offhand if this is a dependency of Browserify, but that deviation would make sense if it leveraged this module. Not sure what @michaelficarra's original use case was, or if he even remembers; it was a long time ago. 😄

Either way, yes, either this should be explained in README.md or reverted. If @michaelficarra's use case is still valid, then maybe a fork or different module is in order...

@michaelficarra

Hmm, I'm not sure how this came about. I just confirmed that both the old (0.10.x) documentation and the implementation have the behaviour that @eventualbuddha outlined. I'd say this should be reverted, but there's probably a use case to prefer npm modules over built-in ones, so an option to preserve that would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment