Improve Windows drive letter URI handling #1816
Conversation
sailingKieler
commented
Feb 21, 2025
sailingKieler
commented
- added additional test cases
… platform regarding drive letter cases * added additional test cases
| @@ -16,6 +16,8 @@ export namespace UriUtils { | |||
| export const joinPath = Utils.joinPath; | |||
| export const resolvePath = Utils.resolvePath; | |||
|
|
|||
| const isWindows = process?.platform === 'win32'; | |||
There was a problem hiding this comment.
This is not portable. Here's a good reference for how to make it work both in Node.js and browser:
https://github.com/eclipse-theia/theia/blob/master/packages/core/src/common/os.ts
There was a problem hiding this comment.
Do we actually need to distinguish the browser case here?
Do we have to deal with windows drive letters in that scenario?
There was a problem hiding this comment.
It doesn't matter. Calling process?.platform will result in an error which will crash the language server in the browser. You need to specifically test for typeof === 'undefined'. See:
langium/packages/langium/src/utils/promise-utils.ts
Lines 19 to 23 in fa8371b
There was a problem hiding this comment.
Additionally, I'd be in favor of calling URI.parse on the input in case the input is a string. It isn't obvious we are expecting a path, and should much rather expect a stringified URI, like we're doing in the other methods. This alleviates te need to perform that kind of normalization ourselves.
There was a problem hiding this comment.
Additionally, I'd be in favor of calling URI.parse on the input in case the input is a string. It isn't obvious we are expecting a path, and should much rather expect a stringified URI, like we're doing in the other methods.
I full agree. I just came up with the proposed change after I realized the painful way that the current implementation has an issue with differing drive letter cases. (It fits very well for a use case in a commercial project.)
Beyond the test cases we have so far, how should relativize act in case of a call like
UriUtils.relativize(
URI.file('C:\\a'),
URI.file('D:\\b')
)
Return undefined? Throwing an exception would be very mean...
There was a problem hiding this comment.
We should follow/emulate whatever node:path is doing there. It will simply return D:\b.
msujew
changed the title
| @@ -36,6 +36,10 @@ export namespace UriUtils { | |||
| if (toParts[0] && upperCaseDriveLetter.test(toParts[0])) { | |||
| toParts[0] = toParts[0].toLowerCase(); | |||
| } | |||
| if (fromParts[0] !== toParts[0]) { | |||
| // in case of different drive letters, we cannot compute a relative path, so... | |||
| return toPath.toLowerCase(); | |||
There was a problem hiding this comment.
Question: Why lowercase the whole string? We can just keep the original capitalization.
sailingKieler
force-pushed
the
cs/uriUtilsDriveLetters
branch
2 times, most recently
from
0422556 to
189a937
Compare
sailingKieler
force-pushed
the
cs/uriUtilsDriveLetters
branch
from
189a937 to
02fd044
Compare
|
Some tests with string input to |
