Support version for installExtension, support uninstall cmd #14298
Conversation
| @@ -81,4 +81,31 @@ export namespace PluginIdentifiers { | |||
| } | |||
| return { id: probablyId.slice(0, endOfName) as UnversionedId, version: probablyId.slice(endOfName + 1) }; | |||
| } | |||
|
|
|||
| const EXTENSION_IDENTIFIER_WITH_VERSION_REGEX = /^([^.]+\..+)@((prerelease)|(\d+\.\d+\.\d+(-.*)?))$/; | |||
There was a problem hiding this comment.
This is unnecessarily restrictve, IMO: all we need to know here is what is before the '@' and what is after the '@'. Why introduce the added complexity? Also, regexes should always have a comment that describes what they are supposed to detect, Because no-one understands Regexes at first sight. If the id does not match the expression, you end up with the exact same result as before.
There was a problem hiding this comment.
This is the same RegEx that is also used in vs-code, I thought it is a good idea to do the same.
I will add a comment to describe what is expected by this RegEx.
| if (!id) { | ||
| throw new Error(nls.localizeByDefault('Extension id required.')); | ||
| } | ||
| if (!PluginIdentifiers.isVersionedId(id)) { |
There was a problem hiding this comment.
I don't think this makes sense from a user's perspective: while technically, we can have multiple versions of the same plugin installed, that's not how it's presented to the user: for them, you either have a plugin installed, or not, independent of the version, so we should probably pass a naked id here.
There was a problem hiding this comment.
PluginServer.uninstall() expects PluginIdentifiers.VersionedId parameter which is typed as ${string}.${string}@${string} that why it the code above makes sense to me.
that's not how it's presented to the user: for them, you either have a plugin installed, or not
The UNINSTALL_EXTENSION command is not presented to the user, but is used in theia/packages/vsx-registry/src/browser/vsx-extension.tsx where the versioned extension id is given.
I would prefer to keep require versioned id here.
| @@ -324,8 +325,10 @@ export class VSXExtension implements VSXExtensionData, TreeElement { | |||
| if (plugin) { | |||
| await this.progressService.withProgress( | |||
| nls.localizeByDefault('Uninstalling {0}...', this.id), 'extensions', | |||
| () => this.pluginServer.uninstall(PluginIdentifiers.componentsToVersionedId(plugin.metadata.model)) | |||
| ); | |||
| async () => { | |||
There was a problem hiding this comment.
Why go through the command here? Why stringify/destringify the id?
There was a problem hiding this comment.
Why go through the command here? Why stringify/destringify the id?
The implementation is moved to a command to resolve #13796 , now calling the new command to reduce code duplicates.
packages/plugin-ext-vscode/src/browser/plugin-vscode-commands-contribution.ts
Outdated
Show resolved
Hide resolved
|
@dhuebner still some open questions on this one. |
|
@tsmaeder |
dhuebner
force-pushed
the
dhuebner/install-with-version-13795
branch
2 times, most recently
from
5dfc23e to
255a41e
Compare
dhuebner
force-pushed
the
dhuebner/install-with-version-13795
branch
from
f9a0ca7 to
ad3cd23
Compare
|
@tsmaeder |
What it does
Allows to use version when executing
workbench.extensions.installExtensioncommand in format<extensionId>@<version>#13795Adds support for
workbench.extensions.uninstallExtensioncommand #13796How to test
Run uninstall on some installed extension.
extension.uninstallnow uses the newworkbench.extensions.uninstallExtensionunder the hood.Install command
workbench.extensions.installExtensionis currently not used in Theia with astringparameter, one need to create a separate command.Follow-ups
Review checklist
Reminder for reviewers