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 joinPath() issue on URIs (#10370) #10434

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Conversation

yannickowow
Copy link
Contributor

@yannickowow yannickowow commented Nov 18, 2021

What it does

Fixes #10370
Fixes #10585

Issues encountered are when calling joinPath on Windows.
For consistency, joinPath calls paths.posix.join instead.
URI then will store all paths with POSIX-like path, and will resolve to filesystem path when calling fsPath.

How to test

Tests are added in types-impl.spec.ts (based on VS Code ones, with no modification). It should work as is.

Review checklist

@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Nov 19, 2021
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 👍

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

Edit: Looks like it was done :)

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@yannickowow do you mind squashing the pull-request so there is only the single commit (removing the merge commit as well)?

@vince-fugnitto vince-fugnitto added the OS/Windows issues related to the Windows OS label Nov 22, 2021
- Use `posix.join` instead of `resolve`
   - Inconsistency between `joinPath` naming convention
   - Use POSIX separator to store path
   - Resolve to windows path when using fsPath
- Added test suite accordingly

Signed-off by: Yanis HAMITI <[email protected]>
@yannickowow
Copy link
Contributor Author

@yannickowow do you mind squashing the pull-request so there is only the single commit (removing the merge commit as well)?

Done !

@colin-grant-work
Copy link
Contributor

@tsmaeder, @RomanNikitenko, does either of you have time to look at this?

@colin-grant-work colin-grant-work self-requested a review January 7, 2022 23:03
@colin-grant-work
Copy link
Contributor

@msujew, since you've been working with path issues and since you have a Windows environment, would you be willing to test this out?

I believe it should also address #10585. I've created a reproduction of that issue on this branch of the sample extensions repo.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Using the example created by @colin-grant-work, I was able to reproduce the issue.

Opening the webview with the change in place correctly loads the webview icon, so I think this addresses both #10370 and #10585.

@colin-grant-work colin-grant-work linked an issue Jan 10, 2022 that may be closed by this pull request
@msujew msujew merged commit e2d8dc4 into eclipse-theia:master Jan 11, 2022
@vince-fugnitto vince-fugnitto added this to the 1.22.0 milestone Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS/Windows issues related to the Windows OS vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webview iconPath not work when using vscode plugin vscode.uri.joinPath returns bad values in plugins
4 participants