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

fix: use platform dependent path for join, resolve, and parse #4

Merged
merged 4 commits into from
Sep 25, 2021

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Sep 24, 2021

This takes advantage of the fact that win32 handles both \ and / as valid separators and therefore can operate safely on a posix path.

  1. resolves Resolve behavior #2 by using path.win32 (resolve)
  2. handles unc paths by using path.win32 (join)
  3. resolves a parse issue where the drive letter was being stripped on windows
  4. normalizes toNamespacedPath as well, for consistency

An observation: this entire library works (passes tests) in reverse using path.win32 and normalising the result (as with 1 & 2 above)

With this PR merged, #3 should be passing.

@danielroe danielroe requested a review from pi0 September 24, 2021 16:15
@danielroe danielroe self-assigned this Sep 24, 2021
@danielroe danielroe changed the title fix: use path.win32 to handle edge cases fix: use path.win32 to handle edge cases Sep 24, 2021
@pi0
Copy link
Member

pi0 commented Sep 25, 2021

Comparing win32 implementation of resolve to the POSIX one there seems a huge difference (and performance impact). In the meantime:

  • I think we can use platform dependant behavior for resolve/join, hoping no one cross runs windows paths in posix env
  • In the longer term, either inline posix.resolve implementation to apply fixes or directly wrap. Since we do not need ALL windows features in a cross-env implementation.

src/index.ts Outdated
}

// toNamespacedPath
export const toNamespacedPath: typeof path.toNamespacedPath = function (p) {
return p
return normalizeWindowsPath(p)
Copy link
Member

Choose a reason for hiding this comment

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

This is also a platform-dependent API that POSIX behavior is to return input as-is....

@pi0 pi0 changed the title fix: use path.win32 to handle edge cases fix: use platform dependent path for join, resolve, and parse Sep 25, 2021
@pi0 pi0 merged commit 059dd64 into main Sep 25, 2021
@pi0 pi0 deleted the fix/win32-edge-cases branch September 25, 2021 07:57
@pi0
Copy link
Member

pi0 commented Sep 25, 2021

I had to revert b90fdaf since seems added edge cases went more :( Let's try when merging test suite for better solution.

pi0 added a commit that referenced this pull request Sep 25, 2021
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.

Resolve behavior
2 participants