Fix: Plugins folders are created with exclamation mark, which breaks webpack

[shahar-h]

What it does

Currently when loading Theia plugin / VS code extension, illegal characters are replaced with ! on plugin folder creation. This is done because filenamify library is used to create legal folder names for plugins, and ! is the default replacement character.

i.e.: Loading theia with the following vsix:
https://github.com/streetsidesoftware/vscode-spell-checker/releases/download/v1.9.0-alpha.1/code-spell-checker-1.9.0-alpha.1.vsix
will cause https!github.com!streetsidesoftware!vscode-spell-checker!releases!download!v1.9.0-alpha.1!code-spell-checker-1.9.0-alpha.1.vsix folder to be created.

In case webpack is used in runtime by some extension it will break because ! is reserved for webpack loaders mechanism.
See:
webpack/webpack#6742
https://github.com/byzyk/webpack/blob/master/schemas/ajv.absolutePath.js#L38

In order to fix this issue _ is passed as a replacement character to filenamify instead of the default !.

How to test

Load Theia with both VS Code extension and Theia plugin from URL:

yarn
cd examples/browser
export THEIA_PLUGINS=https://github.com/streetsidesoftware/vscode-spell-checker/releases/download/v1.9.0-alpha.1/code-spell-checker-1.9.0-alpha.1.vsix,https://github.com/redhat-developer/netcoredbg-theia-plugin/releases/download/v0.0.3/netcoredbg_theia_plugin.theia
yarn start

• https_github.com_streetsidesoftware_vscode-spell-checker_releases_download_v1.9.0-alpha.1_code-spell folder should be created under vscode-unpacked folder.
• https_github.com_redhat-developer_netcoredbg-theia-plugin_releases_download_v0.0.3_netcoredbg_theia_ folder should be created under theia-unpacked folder.

VS Code extension should be activated:

Theia plugin should be activated:

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Shahar Harari [email protected]

[vince-fugnitto]

@shahar-h thank you for the contribution, please take care to squash commits and ensure that they are properly signed (reason for the failed CI check). I'm not sure I understand the use-case, but I do not believe it is common/advised to export plugins in such as way that was described in the pull-request's description.

[akosyakov]

@vince-fugnitto Could you please finish the review and merge if when you think it is good, thank you!

[shahar-h]

I'm not sure I understand the use-case.

The use case is using webpack from VS Code extension to bundle a project.

I do not believe it is common/advised to export plugins in such as way that was described in the pull-request's description.

It's just an easy way to test. The described behavior is also reproduced when deploying a plugin with Deploy Plugin By Id command.

[vince-fugnitto]

I verified using the 'steps to reproduce' and it looks like underscore works fine as a replacement.

I also took the time to verify the 'steps to reproduce' on master and it works correctly for me as well. If there is a bug I'm not sure how to reproduce it, or the 'steps to reproduce' might not be descriptive enough to highlight the buggy behavior.

Nitpick: the commit message is not formatted properly:

[shahar-h]

@vince-fugnitto I can try to create a sample vs code extension in order to show the buggy behavior.
Will it work for you?

[vince-fugnitto]

@vince-fugnitto I can try to create a sample vs code extension in order to show the buggy behavior.
Will it work for you?

I trust you :)
I don't want you to need to spend time on developing an extension, the _ still works correctly for me.

[vince-fugnitto]

I'll give it some time for others to get a chance to review if they like, and merge if there are no objections.

[shahar-h]

@vince-fugnitto Can this be merged or we are still waiting for feedback?

[akosyakov]

@vince-fugnitto please help to merge if you think it is good

[vince-fugnitto]

@vince-fugnitto Can this be merged or we are still waiting for feedback?
@vince-fugnitto please help to merge if you think it is good

I did want others to look but to me _ makes perfect sense as a replacement in all operating systems, and it does not break the current behavior. I'll merge! Thank you for your contribution @shahar-h :)