node
node copied to clipboard
nodejs
path: fix win32.relative() for some Unicode paths
Fixes: https://github.com/nodejs/node/issues/27534
This is an alternative to https://github.com/nodejs/node/pull/27644 that avoids any performance regressions in cases where lowercased versions of paths do not differ in length to the original.
The only minor issue (in practice it may not really matter) is that return values are always returned normalized using canonical composition if the lowercased version differed in length to the original, which may be unexpected if you passed in a path that was normalized using canonical decomposition for example.
Checklist
- [x]
make -j4 test(UNIX), orvcbuild test(Windows) passes - [x] tests and/or benchmarks are included
- [x] commit message follows commit guidelines
Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3494
Alternatively we could ignore additional code points and only compare base symbols, but I'm not sure if that's how Windows/NTFS/etc. compares characters in paths.
@mscdex sorry I made a mistake while looking at this earlier. It should indeed work properly in all cases (if intl is available).
About the polyfill: we could also change the algorithm itself to better reflect what we want in case the length is not identical. Using that would also prevent the edge case mentioned above.
One of these implementations could e.g. look like: "loop over the original first argument and slice out parts, lowercase them and compare them with the other side in case you find a slash." That should be almost as fast as the generic implementation.
ping @mscdex @BridgeAR @jdalton is this ready basically but needs reviews?
@jdalton I think your request changes can now be dismissed?
ping @nodejs/path
Seems like it would fix a crash in my app for users with İ in the file path:
https://github.com/whyboris/Video-Hub-App/issues/533#issuecomment-707890842
@mscdex The CI is failing consistently on very specific jobs, I'm not sure if this needs action or is just flaky CI.
@aduh95 The error in the intl-less build environment makes sense. I don't think we came to a consensus about how to handle that situation. I suggested using a pure JS polyfill for a consistent experience whether intl is available or not. @BridgeAR had a separate suggestion. Maybe someone else has additional input into this or is ok with one of these solutions?
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@mscdex -- could you resolve the conflicting files (currently only lib/path.js)
@jdalton -- is there any reason to delay merging after the merge conflicts are resolved? 🙏
@whyboris I'll be happy to cherry-pick @mscdex's commit, resolve the conflicts and create another PR if they're not available at the moment.
I've been using a custom-built (using #27644) node for almost two years and I really want this officially fixed. 😐
@monoblaine the issue about what to do about intl-less node builds hasn't been resolved yet, so resolving merge conflicts isn't worthwhile at this point
Oh, NOW I understand the problem here. (It took only two years)
A note to my future self and other fast learners like me:
- Node uses ICU to implement internationalization support.
- This significantly increases the file size.
- Therefore node has the option to make a build wihout intl support.
- The changes introduced in this PR is dependent on intl support's existence and that's a problem. (An alternative solution is proposed but not yet discussed.)
@mscdex is this PR still something that can/should land? Is there anything that's currently blocking?
@bnb
is this PR still something that can/should land?
I'm not really the one to ask, I was just providing an alternative solution to an issue.
Is there anything that's currently blocking?
https://github.com/nodejs/node/pull/27662#issuecomment-770283526