Skip to content

Remove UNC Prefix after Canonicalize on Windows - #637

Merged
charlespierce merged 2 commits into
volta-cli:masterfrom
charlespierce:fix_canonicalize
Jan 9, 2020
Merged

charlespierce merged 2 commits into
volta-cli:masterfrom
charlespierce:fix_canonicalize

Conversation

@charlespierce

@charlespierce charlespierce commented Jan 9, 2020 •

Copy link
Copy Markdown
Contributor

Closes #635
Closes #630
Closes #586

Info

Changes

  • Updated all calls to canonicalize to use dunce
  • Dunce inspects the PATH and strips off the UNC prefix if it is safe (following the Windows conventions for when the two paths are equivalent).
    • On non-Windows systems, dunce::canonicalize is an inline passthrough to fs::canonicalize, so Unix remains unaffected.
  • Also cleaned up some redundant clones that Clippy found.

Tested

  • Tested all three bugs caused by UNC paths (gulp-cli, ember-cli, and calling npm i with passthrough.
  • Confirmed that each was reproducible prior to and fixed after the changes.

@chriskrycho chriskrycho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Amazing detective work, and awesome fix! ⚡️

@charlespierce
charlespierce merged commit c712218 into volta-cli:master Jan 9, 2020
@charlespierce
charlespierce deleted the fix_canonicalize branch January 9, 2020 23:58
@wycats

wycats commented Mar 23, 2020

Copy link
Copy Markdown

Great work!

If I recall correctly from when I was helping with this API, the UNC prefix allows the use of very long paths, but that's less of an issue on modern node and modern Windows.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

windows global command issue Infinite loop when running npm i gulp-cli does not appear to work

3 participants