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

Remove slash at the end of extension path #8045

Merged
merged 1 commit into from
Jul 1, 2020
Merged

Conversation

vitaliy-guliy
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy commented Jun 18, 2020

Signed-off-by: Vitaliy Gulyy vgulyy@redhat.com

What it does

Removes the last slash from the extensionPath, which is passed to the extension withing PluginContext.

I didn't find any rules about the slash at the ending of PluginContext.extensionPath on the VS Code API.
A sample helloworld extension running in VS Code prints the path without the slash.

vscode.window.showInformationMessage(`ExtensionPath [${context.extensionPath}]`);

This fix is necessary for some extensions, e.g. Jira and Bitbucket.
Actually we are trying to run this extension inside Che-Theia eclipse-che/che#17081, but faced the problem, that the extension forms wrong URI to some webview resources (URI contains double slashes in the Path).

https://serverj5kpkbom-jwtproxy-webviews.192.168.99.152.nip.io/webview/theia-resource/file/tmp/vscode-unpacked/atlascode-0.0.0.vsix/extension//build/static/js/vendors~atlascodeCreateIssueScreen~atlascodeOnboardingV2~atlascodeSettingsV2~bitbucketIssuePageV2~cr~8893e30c.755b5343.chunk.js

Removing the slash at the end of extensionPath solved the issue.

How to test

Currently it's difficult to test how vanilla Theia works with Jira and Bitbucket extension.
But the reviewer can build Theia and try to run any extension, which use PluginContext.extensionPath.
I don't know such plugin, except Jira&Bitbucket. Any other extensions I know works fine.

Review checklist

Reminder for reviewers

@akosyakov akosyakov added 🤔 needs more info issues that require more info from the author vscode issues related to VSCode compatibility labels Jun 18, 2020
@akosyakov
Copy link
Member

akosyakov commented Jun 18, 2020

Actually we are trying to run this extension inside Che-Theia eclipse-che/che#17081, but faced the problem, that the extension forms wrong URI to some webview resources (URI contains double slashes in the Path).

Do you know where such URIs is created? It could be that such place violating one of rules here: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#uripath Like using path instead of uri and manipulating paths as strings

@vitaliy-guliy
Copy link
Contributor Author

Do you know where such URIs is created? It could be that such place violating one of rules here: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#uripath Like using path instead of uri and manipulating paths as strings

URI is created in the extension, and this extension operates with strings, not with URIs.
But this URI is based on the extensionPath ( API int theia.d.ts )
It should not violate anything.

@@ -92,7 +92,6 @@ export class HostedPluginReader implements BackendApplicationContribution {
if (!pluginPath) {
return undefined;
}
pluginPath = path.normalize(pluginPath + '/');
Copy link
Member

Choose a reason for hiding this comment

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

not sure about removing normalize that's to get rid of .. and . in the path, don't think it is related to trailing slashes

Copy link
Contributor Author

@vitaliy-guliy vitaliy-guliy Jun 23, 2020

Choose a reason for hiding this comment

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

Is it better to resolve the path?
just

pluginPath = path.resolve(pluginPath);

Copy link
Contributor Author

@vitaliy-guliy vitaliy-guliy Jun 23, 2020

Choose a reason for hiding this comment

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

Checked the code again. I think the caller of readPackage must care about the correct path. Further in the code, path.join is used to concatenate the path elements.
Normalizing or resolving the path here is a bit redundant.

Copy link
Member

Choose a reason for hiding this comment

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

@amiramw Could you check please that it does not introduce regressions for #5274? Generally we should use portable URI here to abstract away from OS quirks, but if it does not break i'm fine to merge like that.

@vitaliy-guliy
Copy link
Contributor Author

is there any verdict on this PR?

@akosyakov
Copy link
Member

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@vitaliy-guliy
Copy link
Contributor Author

@akosyakov squashed to one commit

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

smoked tests built-in extensions against both targets, everything seem to be loaded normal

@vitaliy-guliy vitaliy-guliy merged commit dfcfccc into master Jul 1, 2020
@vitaliy-guliy vitaliy-guliy deleted the fix-extension-path branch July 1, 2020 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants