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

correctly resolve paths that are on networked drives #156

Merged
merged 2 commits into from
May 29, 2019

Conversation

NiroSugir
Copy link
Contributor

This is to fix issue #155 where paths don't resolve when it refers to a networked location. The leading backslash gets stripped off which turns paths such as '\\ip\dir' into '\ip\dir'.

@ljharb ljharb force-pushed the networked_drive_fix branch from 844f72e to 4fa2e4a Compare May 2, 2018 05:45
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - could you provide some tests along with this?

@ljharb ljharb force-pushed the networked_drive_fix branch from 4fa2e4a to 895c192 Compare May 16, 2019 05:23
@ljharb
Copy link
Member

ljharb commented May 17, 2019

@GeekMode ping, any interest in completing this PR?

@TimvdEijnden
Copy link

TimvdEijnden commented May 28, 2019

@ljharb I just found out about this issue because circular dependencies are NOT being resolved correctly on Windows by using https://github.com/pahen/madge/ it uses https://github.com/dependents/node-filing-cabinet/ which uses this resolve package. While it does work on macOs.

After investigating for 2 days I've found that #155 is the same issue by debugging the code. After making the same modification the circular dependencies are found. If the author is not continuing with this PR I want to create a new one with tests. This is what came out of my investigation:

Windows with fix from this PR:

dirs = 
0:"C:\Users\tim\myProject\src\node_modules"
1:"C:\Users\tim\myProject\src"
2:"C:\Users\tim\myProject\node_modules"
3:"C:\Users\tim\myProject\src"
4:"C:\Users\tim\node_modules"
5:"C:\Users\tim\myProject\src"
6:"C:\Users\node_modules"
7:"C:\Users\tim\myProject\src"
8:"C:\node_modules"
9:"C:\Users\tim\myProject\src"

Windows with bug:

dirs = 
0:"C:\Users\tim\myProject\src\node_modules"
1:"C:\Users\tim\myProject\src\C:\Users\tim\myProject\src"     <------------------ wrong, does not exist
2:"C:\Users\tim\myProject\node_modules"
3:"C:\Users\tim\myProject\C:\Users\tim\myProject\src"         <------------------ wrong, does not exist
4:"C:\Users\tim\node_modules"
5:"C:\Users\tim\C:\Users\tim\myProject\src"                  <------------------ wrong, does not exist
6:"C:\Users\node_modules"
7:"C:\Users\C:\Users\tim\myProject\src"                      <------------------ wrong, does not exist
8:"C:\node_modules"
9:"C:\C:\Users\tim\myProject\src"                            <------------------ wrong, does not exist

paths =
0:"C:\Users\tim\myProject\src"
1:"C:\Users\tim\myProject"
2:"C:\Users\tim"
3:"C:\Users"
4:"C:\"

modules = 
0:"node_modules"
1:"C:\Users\tim\myProject\src"

macOS with fix from this PR:

dirs = 
0:"/Users/tim/git/myProject/src/node_modules"
1:"/Users/tim/git/myProject/src"
2:"/Users/tim/git/myProject/node_modules"
3:"/Users/tim/git/myProject/src"
4:"/Users/tim/git/node_modules"
5:"/Users/tim/git/myProject/src"
6:"/Users/tim/node_modules"
7:"/Users/tim/git/myProject/src"
8:"/Users/node_modules"
9:"/Users/tim/git/myProject/src"
10:"/node_modules"
11:"/Users/tim/git/myProject/src"

macOS with bug:

dirs = 
0:"/Users/tim/git/myProject/src/node_modules"
1:"/Users/tim/git/myProject/src/Users/tim/git/myProject/src"  <------------------ wrong, does not exist
2:"/Users/tim/git/myProject/node_modules"
3:"/Users/tim/git/myProject/Users/tim/git/myProject/src"      <------------------ wrong, does not exist
4:"/Users/tim/git/node_modules"
5:"/Users/tim/git/Users/tim/git/myProject/src"               <------------------ wrong, does not exist
6:"/Users/tim/node_modules"
7:"/Users/tim/Users/tim/git/myProject/src"                   <------------------ wrong, does not exist
8:"/Users/node_modules"
9:"/Users/Users/tim/git/myProject/src"                       <------------------ wrong, does not exist
10:"/node_modules"
11:"/Users/tim/git/myProject/src"        

paths = 
0:"/Users/tim/git/myProject/src"
1:"/Users/tim/git/myProject"
2:"/Users/tim/git"
3:"/Users/tim"
4:"/Users"
5:"/"

modules = 
0:"node_modules"
1:"/Users/tim/git/myProject/src"

Only thing that remains would be filtering out the duplicates.

@ljharb
Copy link
Member

ljharb commented May 28, 2019

@TimvdEijnden thanks for confirming! please don't make another PR, but if you provide a link to the tests (on your own fork), I'll pull them into the PR so we can bring it in!

@TimvdEijnden
Copy link

@ljharb Yes I'll provide them today.

@TimvdEijnden
Copy link

@ljharb Here they are TimvdEijnden@2e4499b

Once this is merged and published to NPM, I'll update the following projects to update the dependencies:

Then I can finally update the madge depedency in our project 😬

@ljharb
Copy link
Member

ljharb commented May 29, 2019

If all of those are properly using ^, you won't need to make any of the intervening PRs.

@ljharb ljharb force-pushed the networked_drive_fix branch from 895c192 to 01e51f5 Compare May 29, 2019 18:59
@ljharb ljharb force-pushed the networked_drive_fix branch from 01e51f5 to 6a0acc8 Compare May 29, 2019 19:44
@ljharb ljharb merged commit 9e2fd2c into browserify:master May 29, 2019
ljharb added a commit that referenced this pull request May 29, 2019
…ws networked drives

Merge pull request #156 from GeekMode/networked_drive_fix; fixes #155
@TimvdEijnden
Copy link

Yes will check that first once resolve has a new release

@TimvdEijnden
Copy link

@ljharb Do you know when a new release is planned?

@ljharb
Copy link
Member

ljharb commented Jun 3, 2019

No plans, just as soon as i have time to do it.

ljharb added a commit to ljharb/resolve that referenced this pull request Jun 3, 2019
 - [Fix] `node-modules-paths`: correctly resolve paths that are on Windows networked drives (browserify#156)
 - [Fix] Make behavior consistent with require.resolve() for "shadowed" core modules (browserify#148)
@ljharb
Copy link
Member

ljharb commented Jun 6, 2019

v1.11.1 is released.

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.

3 participants