cli: introduce exclude id list for extension-packs

[vince-fugnitto]

What it does

The pull-request introduces a mechanism to exclude extensions by id when resolving builtin extension-packs. The purpose is to allow application developers the flexibility to use extension-packs but still have the control to exclude problematic or unwanted extensions that may be referenced by the pack. Previously, post-processing would be necessary to remove such extensions, for example by deleting the plugin in the plugins directory before a build.

The changes introduce theiaPluginsExcludeIds which lists extensions by id and is used to exclude extensions when resolving extension-packs.

How to test

• update the package.json to the following:

"theiaPlugins": {
  "pack": "https://github.com/vince-fugnitto/vscode-builtin-extensionPack/releases/download/0.0.1/builtin-extension-pack-1.45.1.vsix"
},
"theiaPluginsExcludeIds": [
  "vscode.cpp"
]

• execute the cli command yarn download:plugins
• confirm that the pack is downloaded, and individual extensions are resolved
• confirm that vscode.cpp is logged as skipped
• confirm that vscode.cpp is not present under the plugins folder
• rm -rf plugins/
• repeat the command with theiaPluginsExcludeIds being omitted from the package.json - the command should pass and all referenced extensions should resolve

Example:

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: vince-fugnitto [email protected]

[marcdumais-work]

Looks nice, thanks @vince-fugnitto .

Looking at how this works, I am reminded of another use-case, beyond extension packs. Let's say that there is an extension you like but for some reasons it's not working well ATM, or an extension already in package.json stops working correctly, the only way forward ATM is to not have it mentioned in package.json.

If this PR was made slightly more generic, so that the exclusion works when downloading extensions, not just extension packs, one could instead add it or keep it, and also exclude it, until it works as wanted. Then one needs only remove the exclusion instead of thinking adding the extension.

WDYT?

[alvsan09]

I have tested the CLI and validated that the plugins are successfully excluded.

However, if I then try to start the IDE, I can see in the log entries that those plugins are downloaded to the tmp/.. folder and loaded from there, see:

[vince-fugnitto]

Looking at how this works, I am reminded of another use-case, beyond extension packs. Let's say that there is an extension you like but for some reasons it's not working well ATM, or an extension already in package.json stops working correctly, the only way forward ATM is to not have it mentioned in package.json.

If this PR was made slightly more generic, so that the exclusion works when downloading extensions, not just extension packs, one could instead add it or keep it, and also exclude it, until it works as wanted. Then one needs only remove the exclusion instead of thinking adding the extension.

WDYT?

@marcdumais-work I was thinking of that use-case but I did not see a reason as to why they would list an extension under theiaPlugins but want to exclude it as part of theiaPluginsExcludeIds. It would make more sense to me to simply not reference it in the first place.

Also, we use a arbitrary name: url for theiaPlugins mapping so it means that we would first need to download the plugins under then delete them off the filesystem if we scan the extensions and see an id in the exclude lists.

[marcdumais-work]

However, if I then try to start the IDE, I can see in the log entries that those plugins are downloaded to the tmp/.. folder and loaded from there, see:

Good catch! I guess at runtime we resolve the pack again, and helpfully download anything that might have been missed at build-time :)

[marcdumais-work]

Also, we use a arbitrary name: url for theiaPlugins mapping so it means that we would first need to download the plugins under then delete them off the filesystem if we scan the extensions and see an id in the exclude lists.

I have been thinking that the way we specify plugins by URL is a bit dated - we should maybe support by id, but that's for another PR.

[vince-fugnitto]

Also, we use a arbitrary name: url for theiaPlugins mapping so it means that we would first need to download the plugins under then delete them off the filesystem if we scan the extensions and see an id in the exclude lists.

I have been thinking that the way we specify plugins by URL is a bit dated - we should maybe support by id, but that's for another PR.

@marcdumais-work we could, but it means that we can no longer reference a specific version of a builtin anymore. An application may choose to support a specific builtin version, and not have the framework resolve it to the latest API.

[vince-fugnitto]

@marcdumais-work @alvsan09 the pull-request is ready for another review.
I updated the functionality based on feedback (and refactored the methods for handling extension-packs) to hopefully make it clearer.

I also fixed the issue were extension-packs referenced as builtins are resolved at runtime by removing them from the plugins folder after we complete the resolvement and download of the ids they reference at buildtime.

[marcdumais-work]

@alvsan09 If you can, please re-test and approve - I'd like to get this part of this month's release

[alvsan09]

I have validated the solution, it works as expected! 👍
The code refactoring looks good to me!
Thanks Vince !!