plugin: removed resolveSystemPlugins application prop

[vince-fugnitto]

What it does

The commit updates the resolveSystemPlugins backend application prop to false since it has the better behavior. The flag was initially set to true not to have breaking behavior changes as part of v1.19.0.

The prop is used to not resolve system extension-packs and dependencies at runtime, and instead resolve them at build time.

How to test

• confirm that everything works correctly as today when downloading, searching and installing extensions
• confirm with the following builtins, that they are resolved at build time, and excluded plugins are not in the final app:

"theiaPlugins": {
  "vscode.git": "https://open-vsx.org/api/vscode/git/1.52.1/file/vscode.git-1.52.1.vsix",
  "vscode.markdown-language-features": "https://open-vsx.org/api/vscode/markdown-language-features/1.39.2/file/vscode.markdown-language-features-1.39.2.vsix",
  "vscode-builtin-extensions-pack": "https://open-vsx.org/api/eclipse-theia/builtin-extension-pack/1.50.1/file/eclipse-theia.builtin-extension-pack-1.50.1.vsix",
  "redhat.java": "https://open-vsx.org/api/redhat/java/0.73.0/file/redhat.java-0.73.0.vsix",
  "vscjava.vscode-java-debug": "https://open-vsx.org/api/vscjava/vscode-java-debug/0.30.0/file/vscjava.vscode-java-debug-0.30.0.vsix",
  "vscjava.vscode-java-test": "https://open-vsx.org/api/vscjava/vscode-java-test/0.26.1/file/vscjava.vscode-java-test-0.26.1.vsix",
  "vscjava.vscode-maven": "https://open-vsx.org/api/vscjava/vscode-maven/0.21.2/file/vscjava.vscode-maven-0.21.2.vsix",
  "vscjava.vscode-java-dependency": "https://open-vsx.org/api/vscjava/vscode-java-dependency/0.16.0/file/vscjava.vscode-java-dependency-0.16.0.vsix"
},
"theiaPluginsExcludeIds": [
  "vscode.extension-editing",
  "vscode.microsoft-authentication"
]

• remove the plugins folder (so no builtins are present)
• install the eclipse-theia.builtin-extension-pack through the extensions-view - confirm that the individual plugins are resolved and installed

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]

[vince-fugnitto]

@tsmaeder @RomanNikitenko as part of v1.19.0 I had made the default for the backend application prop true as not to potentially break any behavior you might rely on in che, but the better behavior is to set it to false (where system (builtins) are resolved at build time rather than runtime) as it causes other issues. I wanted to know if you had any feedback on the change, and if you were relying on the previous behavior.

[vince-fugnitto]

@tsmaeder any opinion on not resolving builtin extension-packs and extension-deps at runtime anymore? Alternatively if we agree it should not happen I can remove the flag resolveSystemPlugins altogether.

[tsmaeder]

@vince-fugnitto by "compile time", you mean the "download plugins" step of the build?

[vince-fugnitto]

@vince-fugnitto by "compile time", you mean the "download plugins" step of the build?

@tsmaeder that's correct, builtin plugins (system) would now be handled at the download:plugins step of the build where we can handle extension-packs and extension-deps which respect the excludedIds. The deployer would no longer resolve the builtin plugin's dependencies at it should be done at "compile" time.

[tsmaeder]

@vince-fugnitto I'm trying to understand the implications of this:

1. The "system plugins" are the ones that are in the designated plugins folder, right? (typically theia/plugins).
2. Right now, dependencies of those plugins are resolved at startup (per default) and the necessary plugins downloaded from the appropriate locations, correct?

I assume you want to switch the default for performance reasons, right? Do we know what the performance impact actually is? I'm just wondering why we're not just switching the flag in our example applications?

[vince-fugnitto]

@tsmaeder

1. The "system plugins" are the ones that are in the designated plugins folder, right? (typically theia/plugins).

The system or built-in plugins are the plugins which are present under the application's package.json and ultimately under the plugins/ folder like you've mentioned. The idea would be to use the download:plugins functionality going forward when consuming extension-packs rather than adding plugins to this folder manually when in production.

2. Right now, dependencies of those plugins are resolved at startup (per default) and the necessary plugins downloaded from the appropriate locations, correct?

At the moment, those plugins' dependencies are resolved at startup based on the logic in the deployer yes, but this update will reverse the logic (or completely remove it).

I assume you want to switch the default for performance reasons, right? Do we know what the performance impact actually is? I'm just wondering why we're not just switching the flag in our example applications?

The reason is two-fold, it is both for performance reasons and in order to properly respect the excludeIds.

• performance: having extension-packs resolved at runtime means that the application will start without any plugins present for a time until a compatible version of each extension listed is resolved. This means that users will see no plugins present when the app is started which might not be ideal

• exludedIds: the runtime logic does not respect the excluded-ids list where applications can consume a subset of extension-packs if necessary, for instance if there are problematic extensions based on licenses. The problem is while the download:plugins script does respect it at builtime, the plugin system will try to resolve any plugins referenced which have not been resolved yet (excluded plugins) bypassing the excluded list.

I'm just wondering why we're not just switching the flag in our example applications?

We did update the flag in the examples:

theia/examples/browser/package.json

Lines 15 to 19 in e8b739e

	"backend": {
	"config": {
	"resolveSystemPlugins": false
	}
	}

theia/examples/electron/package.json

Lines 15 to 19 in e8b739e

	"backend": {
	"config": {
	"resolveSystemPlugins": false
	}
	}

The idea to update the flag would be to have a better default for downstream to not have the two issues I listed above.

[marcdumais-work]

Right now, dependencies of those plugins are resolved at startup (per default) and the necessary plugins downloaded from the appropriate locations, correct?

At the moment, those plugins' dependencies are resolved at startup based on the logic in the deployer yes

I think we download dependencies at "built time", yes? The problem is that we re-resolve them at startup?

[vince-fugnitto]

I think we download dependencies at "built time", yes? The problem is that we re-resolve them at startup?

@marcdumais-work correct, the packs for instance are downloaded and resolved at build time, but then re-resolved (not respecting the exclude list) at runtime if anything is not previously resolved.

[paul-marechal]

@tsmaeder in short we are proposing to make "system plugins" static, as we understood that this category of plugins represents builtin-like plugins. The goal is then to prevent Theia applications from doing anything smart to those builtins (just like VS Code doesn't update their builtins unless you upgrade the whole editor).

[tsmaeder]

I'm fine with this change: if consumers have trouble with it, they can put the configuration back or rethink their design. The only question that remains for me is why exludedIds: is honored at build time, but not at run time?

[vince-fugnitto]

I'm fine with this change: if consumers have trouble with it, they can put the configuration back or rethink their design. The only question that remains for me is why exludedIds: is honored at build time, but not at run time?

@tsmaeder I believe we definitely thought of it but did not want to add the breaking changes yet as we would likely need to create an application prop for it (and the plugin list) so it would be accessible to a package like @theia/plugin-ext.

[vince-fugnitto]

@marcdumais-work based on some offline discussions I now removed the application prop altogether, as it should give us the best behavior when consuming system/builtin packs. In the future we can think about further improving the functionality so we are also able to respect the exclude list of plugin ids at buildtime (today) and runtime.

[marcdumais-work]

LGTM, thanks Vince!