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

vsx-registry: implement extension compatibility handling #8191

Merged
merged 2 commits into from
Jan 29, 2021

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Jul 16, 2020

What it does

Fixes: #7464

The following pull-request includes extension compatibility handling when searching, and installing extensions from the open-vsx view. The changes include using the new open-vsx api to determine a compatible version of an extension by parsing the engines.vscode property against our supported API version.

The pull-request includes:

How to test

  1. start the application (plugins should work correctly as before)
  2. the extensions view should search for plugins which are compatible with the current api version
  3. installing plugins should work correctly as before
  4. the new command should display the current api version correctly
  5. using yarn start --vscode-api-version={version}, the api version should properly be updated, searching for extensions should still respect the cli argument

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added the vsx-registry Issues related to Open VSX Registry Integration label Jul 16, 2020
@vince-fugnitto vince-fugnitto self-assigned this Jul 16, 2020
@vince-fugnitto vince-fugnitto force-pushed the vf/vsx-engines branch 4 times, most recently from 805c406 to 2f33ffe Compare July 16, 2020 23:29
@vince-fugnitto
Copy link
Member Author

We currently perform multiple api.getExtension(id) calls on master which makes the optimization we did on open-vsx (to return the engines during the search so we can determine the compatibility) not as useful as it should be.

We should probably adjust the code to properly store an extension's information (obtained through the search) which would then be used to perform different operations such as installation. At the moment, we are performing multiple external requests and always solely by id which is insufficient to obtain the correct version of an extension.

@vince-fugnitto vince-fugnitto force-pushed the vf/vsx-engines branch 2 times, most recently from 9a3d607 to a93170b Compare July 21, 2020 18:15
@vince-fugnitto vince-fugnitto force-pushed the vf/vsx-engines branch 5 times, most recently from 6ffb225 to a7322d4 Compare August 11, 2020 14:20
@vince-fugnitto vince-fugnitto force-pushed the vf/vsx-engines branch 2 times, most recently from 7c7e1e4 to fd4ed00 Compare August 20, 2020 14:54
@vince-fugnitto
Copy link
Member Author

I'll need to address the recent changes to open-vsx to make the pull-request working again.

@vince-fugnitto vince-fugnitto force-pushed the vf/vsx-engines branch 2 times, most recently from 0f5a1c5 to 1f842ea Compare December 3, 2020 13:26
@vince-fugnitto
Copy link
Member Author

@spoenemann are there any plans for the openvsx api to support fetching only compatible extensions from the marketplace, based on a client's expectations? For example, being able to pass the currently supported engines.vscode version as query parameter so that clients (ex: theia-based apps) can only install extensions which are more likely to be supported, and not always "latest".

Currently, the api does not allow clients to easily determine and fetch compatible extensions without multiple queries which raises performance concerns. In addition, each client would have to perform this same extension compatibility fix instead of the server supporting it for all.

@spoenemann
Copy link
Contributor

I added the engines to search results in eclipse/openvsx#144. Since eclipse/openvsx#181 you need to set includeAllVersions=true for this information to be included. Isn't that enough?

@vince-fugnitto
Copy link
Member Author

I added the engines to search results in eclipse/openvsx#144. Since eclipse/openvsx#181 you need to set includeAllVersions=true for this information to be included. Isn't that enough?

@spoenemann I am already using includeAllVersions=true in the changeset when using the query api, but this only gives me all versions of an extension and their url. I still need to include additional handling to iterate over the result list, and fetch individual extensions to determine if they are compatible (which is the blocking issue at the moment for performance, and not querying excessively the registry).

For example: (theme-dracula-at-night):

{
  latest: 'https://open-vsx.org/api/bceskavich/theme-dracula-at-night/latest',
  '2.6.1': 'https://open-vsx.org/api/bceskavich/theme-dracula-at-night/2.6.1',
  '2.6.0': 'https://open-vsx.org/api/bceskavich/theme-dracula-at-night/2.6.0'
}

Perhaps you see something incorrect in my approach, or notice some room for improvement in the api itself.

@spoenemann
Copy link
Contributor

The includeAllVersions=true option is only for the search endpoint, not for query. What is the situation where you would use query and need full compatibility data?

@vince-fugnitto
Copy link
Member Author

The includeAllVersions=true option is only for the search endpoint, not for query. What is the situation where you would use query and need full compatibility data?

Correct, we currently use the query endpoint to fetch compatible extensions when performing a refresh and resolve of an extension, which is used to update the installed version on startup when newer versions of a given plugin are available.

@akosyakov due to this limitation, and the fact it is used for resolve and refresh, do you still believe that there is a performance degradation of the application (ex: startup) which needs to be addressed before the changes can be accepted?

Extensions are installed during the first start of workspaces in Gitpod it can significantly affect first time experience for each new workspace if it is slow.

For this comment, how would you propose the api or functionality be updated in order to ensure only compatible extensions are installed, without the negative performance impact.

@spoenemann
Copy link
Contributor

spoenemann commented Dec 15, 2020

We could add an engine option to the query endpoint so it returns data of the latest version that supports that engine? The value could be something like

    "engine": "vscode@1.52.0"

@vince-fugnitto
Copy link
Member Author

We could add an engine option to the query endpoint so it returns data of the latest version that supports that engine? The value could be something like

    "engine": "vscode@1.52.0"

@spoenemann that's what I was hoping for, to add a new query parameter to make it easier for clients to determine compatibility. Having the engines option would definitely be helpful 👍

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

I totally forgot that there's an includeAllVersions option in the query endpoint. I though you mean the option with the same name at the search endpoint.

By posting a query with includeAllVersions: true, you get an array of the full metadata of all versions of the extension, including the engines of every version. By using this it should not be necessary to send multiple requests per extension.

@vince-fugnitto vince-fugnitto force-pushed the vf/vsx-engines branch 2 times, most recently from 5e4faae to 1b521ea Compare January 6, 2021 14:50
@vince-fugnitto
Copy link
Member Author

@spoenemann thank you for your response on the way forward, I updated the pull-request to do as you suggested.

@vince-fugnitto
Copy link
Member Author

@marechal-p thank you for the feedback, I applied your suggestions 👍

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Thanks, the code looks good to me now.

@marcdumais-work
Copy link
Contributor

@vince-fugnitto I think you wanted to wait after this week's release before merging, so we have next month to fix any issues, before the PR is part of a release. I think it makes sense.

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto I think you wanted to wait after this week's release before merging, so we have next month to fix any issues, before the PR is part of a release. I think it makes sense.

Right, I'll wait for after this week's release, fix conflicts and merge if there are no objections 👍
Thank you for the review Miro!

The following commit updates the logic when searching and installing
vscode extensions from `open-vsx` by verifying for compatibility using
the `engines.vscode` property.

The changes include:
- ability to read the default vscode api version from the `cli` or
  `default`.
- ability to determine the compatible version from the search result
  list.
- checks for compatibility.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
The following commit adds a new command to the `api-samples` to
output the supported vscode api version for development purposes.

The api version is changeable by specifying a cli argument such as:
`yarn start --vscode-api-version={version}`

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto merged commit ef9d9ee into master Jan 29, 2021
@vince-fugnitto vince-fugnitto deleted the vf/vsx-engines branch January 29, 2021 19:37
@github-actions github-actions bot added this to the 1.11.0 milestone Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vsx-registry Issues related to Open VSX Registry Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vsx registry] client should take into account the extension's engines.vscode value
7 participants