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

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

Closed
wants to merge 1 commit into from
Closed

Conversation

michaelficarra
Copy link
Contributor

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

@michaelficarra
Copy link
Contributor Author

Ping @substack.

@michaelficarra
Copy link
Contributor Author

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

@michaelficarra
Copy link
Contributor Author

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

@ghost
Copy link

ghost commented Nov 26, 2013

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

@michaelficarra
Copy link
Contributor Author

@substack: which upstream dependencies do you mean?

@ghost
Copy link

ghost commented Nov 26, 2013

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

@michaelficarra
Copy link
Contributor Author

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

@defunctzombie
Copy link

@substack was I suppose to update/fix something? Maybe I have forgotten.
On Nov 26, 2013 3:53 PM, "James Halliday" [email protected] wrote:

Waiting for @defunctzombie https://github.com/defunctzombie to update
browser-resolve then I can upgrade module-deps.


Reply to this email directly or view it on GitHubhttps://github.com//pull/30#issuecomment-29332855
.

@ghost
Copy link

ghost commented Nov 26, 2013

@defunctzombie can you update browser-resolve to use [email protected]?

@defunctzombie
Copy link

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

@eventualbuddha
Copy link

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
Copy link

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
Copy link
Contributor Author

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
Labels
None yet
Development

Successfully merging this pull request may close these issues.

fails to resolve packages with the same name as core modules
4 participants