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

Deep references no longer working #76

Closed
matthewp opened this issue Mar 11, 2015 · 3 comments · Fixed by #77
Closed

Deep references no longer working #76

matthewp opened this issue Mar 11, 2015 · 3 comments · Fixed by #77

Comments

@matthewp
Copy link
Contributor

#62 added the ability to reference modules deeply with "browser". It seems that functionality is no longer working. For example deep/ref will work but very/deep/ref will not.

I think the reason is this regular expression: https://github.com/substack/node-resolve/blob/master/lib/async.js#L109

I assume this is intended to chop off the back but instead is chopping off the front. /Users/matthew/foo becomes matthew/foo. I'll submit a new test and fix the regex.

@matthewp
Copy link
Contributor Author

I'm unsure about the intent of this code. This is where the bug occurs:

dir.replace(/[\\\/]*[^\\\/]+[\\\/]*/, ''), cb

This will cause recursion like:

/Users/matthew/Projects/node-resolve/test/precedence/aaa
matthew/Projects/node-resolve/test/precedence/aaa
Projects/node-resolve/test/precedence/aaa

I would assume what you really want is:

/Users/matthew/Projects/node-resolve/test/precedence/aaa
/Users/matthew/Projects/node-resolve/test/precedence
/Users/matthew/Projects/node-resolve/test

But in that case why a regex and not path.dirname? That leads me to believe my assumption about what this code is trying to do might be wrong. I updated this regex with one that does what I think it should do but that breaks a couple of other tests. But I can't imagine the behavior that is currently happening is intentional.

Could you give some insight as to why you implemented it this way @substack?

matthewp added a commit to matthewp/node-resolve that referenced this issue Mar 11, 2015
This test shows that deep references do not work currently.
@matthewp
Copy link
Contributor Author

I've added a breaking test. Using a different regex or path.dirname will fix this issue but cause other tests to fail which seem to depend on this bug. I'm going to try and investigate what they are doing.

@matthewp
Copy link
Contributor Author

Ok, after looking at the failing tests they were checking if a pkg was found (expecting it not to be) but I don't think this is a necessary part of the tests. The precedence test is verifying that precedence is given to folder/index.js and that is working. Submitting a PR.

matthewp added a commit to matthewp/node-resolve that referenced this issue Mar 11, 2015
This fixes async resolution when looking for a package.json file.
Previously a regex was being used that was not allowing the recursive
behavior the code is attempting. Replacing with path.dirname fixes the
issue.

Fixes browserify#76
@ghost ghost closed this as completed in #77 Mar 15, 2015
matthewp added a commit to matthewp/node-browser-resolve that referenced this issue Mar 16, 2015
This upgrades the resolve dependency which fixed
browserify/resolve#76
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

1 participant