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.

Sign up for GitHub

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

Improve Windows drive letter URI handling #1816

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Improve Windows drive letter URI handling #1816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec492d3
langium: another refinement in 'uri-utils.ts' specific to the Windows…
sailingKieler Feb 21, 2025
02fd044
more supplements
sailingKieler Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions packages/langium/src/utils/uri-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ export namespace UriUtils {
export const joinPath = Utils.joinPath;
export const resolvePath = Utils.resolvePath;

const isWindows = process?.platform === 'win32';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we actually need to distinguish the browser case here?
Do we have to deal with windows drive letters in that scenario?

Copy link
Member

Choose a reason for hiding this comment

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

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

if (typeof setImmediate === 'undefined') {
setTimeout(resolve, 0);
} else {
setImmediate(resolve);
}

Copy link
Member

@msujew msujew Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sailingKieler sailingKieler Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

We should follow/emulate whatever node:path is doing there. It will simply return D:\b.


export function equals(a?: URI | string, b?: URI | string): boolean {
return a?.toString() === b?.toString();
}
Expand All @@ -25,6 +27,17 @@ export namespace UriUtils {
const toPath = typeof to === 'string' ? to : to.path;
const fromParts = fromPath.split('/').filter(e => e.length > 0);
const toParts = toPath.split('/').filter(e => e.length > 0);

if (isWindows) {
const upperCaseDriveLetter = /^[A-Z]:$/;
if (fromParts[0] && upperCaseDriveLetter.test(fromParts[0])) {
fromParts[0] = fromParts[0].toLowerCase();
}
if (toParts[0] && upperCaseDriveLetter.test(toParts[0])) {
toParts[0] = toParts[0].toLowerCase();
}
}

let i = 0;
for (; i < fromParts.length; i++) {
if (fromParts[i] !== toParts[i]) {
Expand Down
18 changes: 18 additions & 0 deletions packages/langium/test/utils/uri-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,24 @@ describe('URI Utils', () => {
expect(UriUtils.relative(from, to)).toBe('../d.txt');
});

test.skipIf(process.platform !== 'win32')('relative path in parent directory win32, uppercase drive letters', () => {
const from = URI.file('C:\\a\\b');
const to = URI.file('C:\\a\\d.txt');
expect(UriUtils.relative(from, to)).toBe('../d.txt');
});

test.skipIf(process.platform !== 'win32')('relative path in parent directory win32, mixed drive letter cases 1', () => {
const from = URI.file('C:\\a\\b');
const to = URI.file('c:\\a\\d.txt');
expect(UriUtils.relative(from, to)).toBe('../d.txt');
});

test.skipIf(process.platform !== 'win32')('relative path in parent directory win32, mixed drive letter cases 2', () => {
const from = URI.file('c:\\a\\b');
const to = URI.file('C:\\a\\d.txt');
expect(UriUtils.relative(from, to)).toBe('../d.txt');
});

test('relative path in sub directory', () => {
const from = URI.file('/a');
const to = URI.file('/a/b/c.txt');
Expand Down