vsx: add 'engines' handling #8063

Anasshahidd21 · 2020-06-20T03:07:26Z

What it does

The following commit adds additional handling and functionality to the vsx-registry extension in order to obtain the latest compatible version of an extension (instead of the latest) by verifying it's engines.vscode property. A compatible extension is defined as one which respects the default API version of the framework, and is useful to narrow down the list of potential extensions to those advertised as working correctly. The changes are also aligned with vscode which perform similar checks.

The pull-request includes:

a new API to get the latest compatible version of an extension
additional handling to process and validate the engines.vscode property
updates to the search, resolve, and install of extensions (to use the compatible version)
updates to the vsx-editor to display the version (similarly to vscode)

How to test

For test purposes, the prettier extension was used:

Prettier Version	Prettier `engines.vscode`	`VSCODE_DEFAULT_API`
`5.0.1`	`^1.41.0` (link)	`1.44.0`
`3.20.0`	`^1.34.0` (link)	`1.40.0`
None	None	`1.20.0`

start the application, open the 'extensions view' and search for prettier
the listed version should satisfy the version present in the above table
downgrade the VSCODE_API_VERSION (1.40.0) and rebuild the changes
repeat steps 1 and 2
downgrade the VSCODE_API_VERSION (1.20.0) and rebuild the changes
prettier should no longer appear in the search
verify that installation works correctly, and that the editor displays the correct version

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

Fixes: eclipse-theia#7464 The following commit updates the `vsx-registry` to perform checks on vscode extensions to ensure that they meet the `engines.vscode` requirement that the default API version declares. The pull request: - fetches the latest compatible extension from the marketplace (instead of the latest) - adds handling to determine if a `engines.vscode` is valid - includes the version to the `vsx-editor`. Co-authored-by: Kaiyue Pan <kaiyue.pan@ericsson.com> Co-authored-by: Anas Shahid <muhammad.shahid@ericsson.com> Co-authored-by: vince-fugnitto <vincent.fugnitto@ericsson.com> Signed-off-by: Kaiyue Pan <kaiyue.pan@ericsson.com> Signed-off-by: Anas Shahid <muhammad.shahid@ericsson.com>

akosyakov

I have performance concerns with the approach. For each search instead of doing one request, we do several additional requests for each extension. The same during the install and look up of an individual extension.

akosyakov · 2020-06-22T06:34:21Z

packages/vsx-registry/src/browser/vsx-extensions-model.ts

@@ -49,6 +50,9 @@ export class VSXExtensionsModel {
    @inject(VSXExtensionsSearchModel)
    readonly search: VSXExtensionsSearchModel;

+    @inject(VSXEnvironment)
+    protected readonly environment: VSXEnvironment;


akosyakov · 2020-06-22T06:36:59Z

packages/vsx-registry/src/common/vsx-utils.ts

+ *
+ * @returns `true` if the engine satisfies the API version.
+ */
+export function isEngineValid(engines: string[]): boolean {


https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#di-function-export

please don't create many utils file, it tells i don't know to which class this responsibility should belong, so here is a sink file for all such functions. Please add it as a method to some appropriate class. In this case it can just stay in VSXReigstryApi.

akosyakov · 2020-06-22T06:38:11Z

packages/vsx-registry/src/browser/vsx-extensions-model.ts

@@ -139,12 +143,18 @@ export class VSXExtensionsModel {
            const searchResult = new Set<string>();
            for (const data of result.extensions) {
                const id = data.namespace.toLowerCase() + '.' + data.name.toLowerCase();
+                const extension = await this.api.getLatestCompatibleExtension(id);


it looks expensive to query for each found extension

akosyakov · 2020-06-22T06:41:21Z

packages/vsx-registry/src/common/vsx-registry-api.ts

+     */
+    async getLatestCompatibleExtension(id: string): Promise<VSXExtensionRaw | undefined> {
+        const extension = await this.getExtension(id);
+        for (const extensionVersion in extension.allVersions) {


I wonder should not it be rather solved in the registry? For each extension we will do potentially many server requests. I imagine there should be one request to fetch an extension by id for such vscode engine.

I wonder should not it be rather solved in the registry? For each extension we will do potentially many server requests. I imagine there should be one request to fetch an extension by id for such vscode engine.

It would be good to enhance the registry API to also accept the engines.vscode as an optional query parameter and only return the latest compatible result.

Would you be fine to improve the API to accept such a query parameter and add the logic to determine the latest compatible extension directly in the registry?

cc @spoenemann

I wonder if e.g. VScodium, that now uses Open VSX though an adapter, is able to pick a version of an extension to install, that is appropriate to it's own API version? Presumably this would be done client-side too? Potentially in a better optimised way?

marcdumais-work · 2020-06-22T16:24:06Z

I have performance concerns with the approach. For each search instead of doing one request, we do several additional requests for each extension. The same during the install and look up of an individual extension.

As per discussions on this PR, the ideal solution would be server-side and avoid playing ping-pong like the client-side version in this PR does. Depending on the ETA of required server-side enhancements, it could be worthwhile to still merge a simpler version of this PR, and update in a follow-up PR to use server-side checks when available.

The performance concerns are valid, with the current client-side implementation. To lessen performance impacts, I propose we, for Extensions Search:

display results without checking for compatibility, potentially showing extensions that have no API-compatible version.
when an extension is selected by user, display the README of the latest version, without regard for API compatibility
abandon other "nice to have" features that add requests when performing extension search

For Extension Install I think we'll need multiple requests, until we have server support. But for the majority of extensions, the latest version will be compatible, so we might first check if latest is compatible, and if not, trigger a more costly search for a compatible version.

@vince-fugnitto WDYT?

vince-fugnitto · 2020-06-22T16:42:44Z

I have performance concerns with the approach. For each search instead of doing one request, we do several additional requests for each extension. The same during the install and look up of an individual extension.

As per discussions on this PR, the ideal solution would be server-side and avoid playing ping-pong like the client-side version in this PR does. Depending on the ETA of required server-side enhancements, it could be worthwhile to still merge a simpler version of this PR, and update in a follow-up PR to use server-side checks when available.

The performance concerns are valid, with the current client-side implementation. To lessen performance impacts, I propose we, for Extensions Search:

display results without checking for compatibility, potentially showing extensions that have no API-compatible version.

when an extension is selected by user, display the README of the latest version, without regard for API compatibility

abandon other "nice to have" features that add requests when performing extension search

For Extension Install I think we'll need multiple requests, until we have server support. But for the majority of extensions, the latest version will be compatible, so we might first check if latest is compatible, and if not, trigger a more costly search for a compatible version.

@vince-fugnitto WDYT?

I would be fine to implement the simple minimal solution first, and when the registry is ready to support the engines.vscode query to return the latest compatible version can we also update the search and align the rest of the functionality.

As an improvement I think we can notify end-users (notification) whenever we need to install an older version (compatible) of an extension so at least we are transparent. It will avoid confusion of why they might see an older extension version than the one listed in the view and readme.

Anasshahidd21 · 2020-06-22T18:51:21Z

Thank you @akosyakov @vince-fugnitto @marcdumais for your reviews and suggestions.

I do agree, it is expensive to have multiple API calls during the search. If the server could have a check for the engine, then it will be great as it can return the compatible version(s) if any.
However, if we want a simpler solution for the time before the changes are made on the server-side(if they are meant to be), then we can just omit API calls on search, and let it be the way it is being done in master.

What we can keep from this PR is, when we open the extension in the editor(view), only then we update the selected extension to take the latest compatible version. This way we don't have to do API calls for all the extensions found on the search result, but only on the one which we desire to install. This will help us install the right version, and it will be a lot less expensive than the currently proposed implementation.

What do you guys think of this approach?

akosyakov · 2020-06-23T06:45:38Z

in https://github.com/eclipse-theia/theia/pull/8063/files#r443640909

@marcdumais-work did a very good point, the difference it that for VS Code we return engine associated with each version, so there is not need for additional requests.

but only on the one which we desire to install

it still can make install slow to loop over all versions for each extension.

vince-fugnitto · 2020-06-23T15:02:55Z

in https://github.com/eclipse-theia/theia/pull/8063/files#r443640909

@marcdumais-work did a very good point, the difference it that for VS Code we return engine associated with each version, so there is not need for additional requests.

@akosyakov do we have access to this same API, do you have example? If so, we can easily determine the valid version and perform the changes client side (like vscode).

marcdumais-work · 2020-06-23T16:38:59Z

it still can make install slow to loop over all versions for each extension.

It should be about a single extension at this point, that the user has selected, presumably to get more info before deciding to install or not. He may do the same with another extension in 10s, but it's only done for one extension at a time.

@Anasshahidd21 - I think we should to confirm what a worse-case situation would be like. e.g. will the user notice a delay if we need to search deep for a compatible version of an extension vs if latest is compatible. Maybe have little videos to show us?

Anasshahidd21 · 2020-06-23T17:54:17Z

@Anasshahidd21 - I think we should to confirm what a worse-case situation would be like. e.g. will the user notice a delay if we need to search deep for a compatible version of an extension vs if latest is compatible. Maybe have little videos to show us?

The test extension I used was RedHat Java, the API link for it is here.

Considering latest version is compatible - This PR

Time elapsed: 106ms.

Considering latest version is not compatible- This PR

Time elapsed: 379ms.
Downgraded the version for the API to 1.41.0
Goes 5 versions deep (v0.59.1)

Considering latest version is compatible - MASTER

Time elapsed: 53ms.

Analysis

Based on the analysis with the different use cases, we can say the complexity is almost linear in this case. In case of extensions with versions greater than 15-20, it might take a longer time for it to install.
Looking at extensions in the open-vsx API, most of them are compatible with our current version.

@marcdumais-work

marcdumais-work · 2020-06-23T20:57:42Z

Thanks Anas.

So:
0 version checked for API compat: ~50ms
1 version checked for API compat: ~110ms
5 version checked for API compat: ~380ms

It makes sense that this would grow about linearly with the number of versions checked for compatibility - the checks does not become more costly for subsequent versions. We also have a baseline time of ~50ms to display the README without any version compatibility checking. We can approximate this so:

t =~ 50ms + 60ms * n, where n = number of versions of an extension that is checked for API compatibility

We can extrapolate that if there needs to be 10 versions checked before finding a compatible one. it will take:
t =~ 50 + 60 * 10
t =~ 650ms

In 1 second, we can search how many versions?
1000ms =~ 50ms + 60ms * n
950 ms =~ 60ms * n
n = 950ms / 60 ms
n = ~16

Context

the latest version of most extension will be API compatible. Estimate: 9/10 (using Theia master)
most extensions are not published that often. This means we will not need to go back many more versions before finding a compatible one, most of the time.
most users will not notice/care if it takes 500ms to open the extension's README or to start the install of an extension
- it will be lost in the noise (network issue? Something else momentarily competing for CPU/RAM/WIFI right now? )
at the "a few seconds" threshold, users will notice and care, specially if it happens often.

Potential Optimization

@marechal-p suggests considering a binary search algorithm. For example, after checking one or a few most recent versions of an extension unsuccessfully and if we know there are many more to potentially check, we could then switch to such an algorithm, that will make the remaining search O(log n) vs current linear. This would essentially limit the damages, in rare cases where there might be many versions of an extension published, with potentially none that's compatible.
- one caveat: such an algorithm works well on sorted lists. In rare cases, there might be extensions that are published with a lower API version required than the previous version. If we have such a thing in the registry, a binary search algo might reach wrong conclusions, if such a discontinuity happens to be checked.

akosyakov · 2020-06-24T07:20:05Z

We could make a PR to open vsx registry to expose vscode engine for each version as well, redeploy and make use of it?

vince-fugnitto · 2020-06-24T15:13:49Z

We could make a PR to open vsx registry to expose vscode engine for each version as well, redeploy and make use of it?

@akosyakov I think exposing the engine as part of the version (presumably the allVersions field) would be great, we can then update the client to make use of it without the need to perform additional requests.

akosyakov · 2020-06-27T07:30:07Z

There is even already code doing it for VS Code adapter: https://github.com/eclipse/openvsx/blob/cae067ede155e36e5bf48c4c93e97074bfac1256/server/src/main/java/org/eclipse/openvsx/adapter/VSCodeAdapter.java#L359

vince-fugnitto · 2020-07-09T12:45:20Z

Closing. The goal would be first to update the open-vsx API first (to handle returning the engines.vscode property) so that clients will not need to query the server multiple times unnecessarily to determine the latest compatible extension.

Upstream Issue:

api: update general api to pass engines along with extension version eclipse/openvsx#140

Anasshahidd21 force-pushed the vsx-engine-handling branch 3 times, most recently from 48ec958 to fac4a68 Compare June 20, 2020 03:31

vince-fugnitto added vscode issues related to VSCode compatibility vsx-registry Issues related to Open VSX Registry Integration labels Jun 20, 2020

vince-fugnitto requested review from akosyakov and marcdumais-work June 20, 2020 14:47

Anasshahidd21 force-pushed the vsx-engine-handling branch from fac4a68 to b1ed4d1 Compare June 20, 2020 16:32

akosyakov reviewed Jun 22, 2020

View reviewed changes

vince-fugnitto mentioned this pull request Jun 26, 2020

vsx-registry: fix the 'licenseUrl' link #8095

Merged

1 task

vince-fugnitto mentioned this pull request Jul 9, 2020

api: update general api to pass engines along with extension version eclipse/openvsx#140

Closed

vince-fugnitto closed this Jul 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vsx: add 'engines' handling #8063

vsx: add 'engines' handling #8063

Anasshahidd21 commented Jun 20, 2020 •

edited by vince-fugnitto

Loading

akosyakov left a comment •

edited

Loading

akosyakov Jun 22, 2020

akosyakov Jun 22, 2020

akosyakov Jun 22, 2020

akosyakov Jun 22, 2020

vince-fugnitto Jun 22, 2020

akosyakov Jun 22, 2020

marcdumais-work Jun 22, 2020

marcdumais-work commented Jun 22, 2020 •

edited

Loading

vince-fugnitto commented Jun 22, 2020

Anasshahidd21 commented Jun 22, 2020

akosyakov commented Jun 23, 2020 •

edited

Loading

vince-fugnitto commented Jun 23, 2020

marcdumais-work commented Jun 23, 2020

Anasshahidd21 commented Jun 23, 2020 •

edited

Loading

marcdumais-work commented Jun 23, 2020

akosyakov commented Jun 24, 2020

vince-fugnitto commented Jun 24, 2020

akosyakov commented Jun 27, 2020

vince-fugnitto commented Jul 9, 2020

vsx: add 'engines' handling #8063

vsx: add 'engines' handling #8063

Conversation

Anasshahidd21 commented Jun 20, 2020 • edited by vince-fugnitto Loading

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov left a comment • edited Loading

Choose a reason for hiding this comment

akosyakov Jun 22, 2020

Choose a reason for hiding this comment

akosyakov Jun 22, 2020

Choose a reason for hiding this comment

akosyakov Jun 22, 2020

Choose a reason for hiding this comment

akosyakov Jun 22, 2020

Choose a reason for hiding this comment

vince-fugnitto Jun 22, 2020

Choose a reason for hiding this comment

akosyakov Jun 22, 2020

Choose a reason for hiding this comment

marcdumais-work Jun 22, 2020

Choose a reason for hiding this comment

marcdumais-work commented Jun 22, 2020 • edited Loading

vince-fugnitto commented Jun 22, 2020

Anasshahidd21 commented Jun 22, 2020

akosyakov commented Jun 23, 2020 • edited Loading

vince-fugnitto commented Jun 23, 2020

marcdumais-work commented Jun 23, 2020

Anasshahidd21 commented Jun 23, 2020 • edited Loading

marcdumais-work commented Jun 23, 2020

Context

Potential Optimization

akosyakov commented Jun 24, 2020

vince-fugnitto commented Jun 24, 2020

akosyakov commented Jun 27, 2020

vince-fugnitto commented Jul 9, 2020

Anasshahidd21 commented Jun 20, 2020 •

edited by vince-fugnitto

Loading

akosyakov left a comment •

edited

Loading

marcdumais-work commented Jun 22, 2020 •

edited

Loading

akosyakov commented Jun 23, 2020 •

edited

Loading

Anasshahidd21 commented Jun 23, 2020 •

edited

Loading