feat(browser-only): Extensions support#17122
feat(browser-only): Extensions support#17122kachurun wants to merge 17 commits intoeclipse-theia:masterfrom
Conversation
Removed double-click handler that caused folders to auto-open (eclipse-theia#15868)
6fbef76 to
d33cf95
Compare
9dfa9b7 to
13215ba
Compare
| import * as fs from '@theia/core/shared/fs-extra'; | ||
| import { ApplicationPackage } from '@theia/core/shared/@theia/application-package'; |
There was a problem hiding this comment.
This is required by ESLint since @theia/core is now a dependency of application-manager
There was a problem hiding this comment.
I would like to avoid a dependency from the build package to the runtime packages of Theia
There was a problem hiding this comment.
Changes here are needed to lazy-load snippets on demand instead of on Theia load. Otherwise, it loads all snippets of all languages from plugins on page load
There was a problem hiding this comment.
Does that mean that the snippets are now "slower" when shown?
| const existing = this.themeService.getTheme(id!); | ||
|
|
||
| if (existing.id === id && existing.editorTheme) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do not load the theme if it already exists in themeService. Otherwise, it loads all theme files each time Theia loads.
There was a problem hiding this comment.
But the ThemeService is scoped to Theia? So whenever Theia loads we load here anyway. Do we load themes multiple times by accident? What are the steps to reproduce the issue?
| return `${pluginId}/${Localization.transformKey(scope)}/${key}`; | ||
| } | ||
|
|
||
| // Localization logic for `package.json` entries |
There was a problem hiding this comment.
Helpers were just moved to plugin-package-localization.ts
| if (contributions.themes && contributions.themes.length) { | ||
| const pending = {}; | ||
| for (const theme of contributions.themes) { | ||
| pushContribution(`themes.${theme.uri}`, () => this.monacoThemingService.register(theme, pending)); |
There was a problem hiding this comment.
Pending was moved to a MonacoThemingService internal property instead of being drilled through all methods in it
02d4bfa to
12470af
Compare
d6c51cd to
1bfa4af
Compare
There was a problem hiding this comment.
I am creating an Inversify DI container here during the build to load the classes we normally use with the Node backend at runtime
There was a problem hiding this comment.
Conceptually I am fine with creating a DI container for the build
There was a problem hiding this comment.
In this file I do:
- Find the closest package with theiaPluginsDir specified
- Copy those plugins to dist/frontend/hostedPlugin
- Localize package.json to skip package.nls.json loading at runtime
- Create a DeployerPlugin shape from plugin/package.json
- Normalize paths
- Generate dist/frontend/hostedPlugin/list.json with all deployed plugins
There was a problem hiding this comment.
Generally speaking this makes sense to me
| // If package contains localizable strings, load the translations and localize the manifest | ||
| if (hasLocalizableStrings(manifest)) { | ||
| const translations = await loadTranslations(pluginModel); | ||
| if (Object.keys(translations).length > 0) { | ||
| return localizeWithResolver(manifest, key => translations[key]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Browser-only translations in package.json are already translated, so there is no reason to load them
| // do not start backend plugins for browser-only | ||
| if (environment.browserOnly.is()) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Attempting to run backend plugins in a browser-only environment leads to an endless await from onWillSave (something like this), so files just never save. Maybe it's not the right place, but it solves the issue.
There was a problem hiding this comment.
We could have another fix: backend plugins could be excluded from the very beginning in the browser-only plugin list, i.e. at build time.
| async localizeManifest(pluginPath: string, manifest: PluginPackage): Promise<PluginPackage> { | ||
| const locale = this.localizationProvider.getCurrentLanguage(); | ||
| const translations = await loadPackageTranslations(pluginPath, locale); | ||
| return localizePackage(manifest, translations, (_, defaultVal) => defaultVal) as PluginPackage; | ||
| } |
There was a problem hiding this comment.
I can do this purely in the application manager where I use it, but then there will be a lot of duplicate code, but here everything is in place
| try { | ||
| const pluginModel = plg.model; | ||
| const pluginLifecycle = plg.lifecycle; | ||
| const pluginFolder = FileUri.fsPath(pluginModel.packageUri); |
There was a problem hiding this comment.
file:///home/abc/... -> /home/abc/...
There was a problem hiding this comment.
Code moved from packages/plugin-ext/src/hosted/node/hosted-plugin-localization-service.ts so I kept Original Copyright
There was a problem hiding this comment.
Code also moved from packages/plugin-ext/src/hosted/node/plugin-reader.ts so copyright is original, added only .mjs extension in list, just in case
| const commandDef = typeof command === 'string' ? { id: command } : command; | ||
|
|
||
| if (handler && commandIsDeclaredInPackage(commandDef.id, plugin.rawModel)) { | ||
| return commandRegistry.registerHandler(commandDef.id, handler, thisArg); |
There was a problem hiding this comment.
To simplify the code and fix the issue when commands are registered twice and warn
There was a problem hiding this comment.
Seems like a drive-by fix? Could also be a separate PR
| const ownURI = new Endpoint().getRestUrl(); | ||
| const fullURI = ownURI.parent.resolve(PluginPackage.toPluginUrl(plugin.model, '')); |
There was a problem hiding this comment.
browser-only: Allow serving static files from a base path other than /, for example /ide/bundle.js, /ide/hostedPlugin/*, etc.
| const packageRoot = (URI.parse(plugin.model.packageUri).path || plugin.model.packageUri).replace(/\\/g, '/'); | ||
| const absolutePath = normalize(arg.startsWith('/') ? arg : resolve(packageRoot, arg)); | ||
| const relativePath = relative(packageRoot, absolutePath); | ||
|
|
||
| return PluginPackage.toPluginUrl(plugin.model, relativePath); |
There was a problem hiding this comment.
To resolve icons relative to the plugin package for any environment and with a non-standard base path for browser-only
There was a problem hiding this comment.
This was completely broken in a browser-only environment, so any plugins that implement quick open menus just fail
There was a problem hiding this comment.
Plugins can't start if terminals are not initialized, so we must have these mocks
| get: (_target: object, name: ('_empty' | keyof typeof theia)) => { | ||
| const plugin = pluginsModulesNames.get(ctx.frontendModuleName); | ||
|
|
||
| if (!defaultApi) { | ||
| defaultApi = apiFactory(emptyPlugin); | ||
| const api = plugin | ||
| ? pluginsApiImpl.get(plugin.model.id) | ||
| : (defaultApi ?? (defaultApi = apiFactory(emptyPlugin))); | ||
|
|
||
| // require('vscode') resolves to `theia._empty` return full API so plugin gets the whole object | ||
| if (name === '_empty') { | ||
| return api; | ||
| } | ||
|
|
||
| return defaultApi; | ||
| // Direct access (theia.l10n, theia.commands, etc.) return that property from the API | ||
| return api![name]; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This fixes l10n in plugins and potentially several other issues, because plugins now receive the correct plugin context in browser-only environments.
The previous implementation did not make much sense because name was always '_empty' rather than the actual plugin name. The plugin name can be derived from ctx.frontendModuleName instead.
This also makes loading a specific key, rather than the entire API object, work correctly.
There was a problem hiding this comment.
Localization should now basically work. I verified the mechanism using an English → English override: bundle.l10n..json is loaded and parsed, the correct plugin context is resolved, and overridden strings are returned. This should work for other languages as well. However, plugin manifests are not localized yet, so the practical impact is currently limited.
|
@kachurun You are still working on this PR. Can you indicate in which state this PR is, e.g. draft, functionally complete, open-issues, polished and ready to merge etc., and correspondingly what level of review you are currently looking for? |
|
@sdirix |
…into browser-only-ext
|
@sdirix I have tested it in my project for a week, and it works great, so it's ready for a complete review. I'm not going to change anything in the code. The only problems I have are not related to Theia. It's just that the TS LSP extension works in partial mode in a browser-only environment, but it doesn't work anywhere: https://code.visualstudio.com/docs/nodejs/working-with-javascript#_partial-intellisense-mode |
There was a problem hiding this comment.
Thank you for this contribution. I tested it on my machine and it works! It's a great achievement to bring this into Theia Browser-Only and it's even convenient to use. Good job!
Generally speaking it would be good to reduce the PR to the bare minimum of what is required to bring the feature in. Any drive-by fix which is not related to the plugins itself should be moved out into a separate PR. I already commented two of these, but I think there are more like the command popup not working. These could be nice small PRs which are easy to review and would already be in the Theia codebase if opened separately.
This leaves two major points for me:
- If the changes were mainly restricted to browser-only code, then I would have an easier time to approve. However as many parts of a core feature of Theia are touched, this will take more time to review. It would be good to restrict these changes to the bare minimum. For example it's unclear to me whether we really need to remove the deprecated field for this change.
- I am unsure about adding Theia runtime packages as dependency to the dev package. At the moment we already have dependencies the other way around, with this they become fully intertwined. To me it seems it would be cleaner to extract the logic into an own package which is consumed by both. However of course this would increase the scope of the PR which is what I argued against before.
@msujew Do you have an opinion on the plugin discovery mechanism and how it should be consumed as a build step for browser-only?
| "@theia/core": "1.69.0", | ||
| "@babel/core": "^7.10.0", | ||
| "@babel/plugin-transform-classes": "^7.10.0", | ||
| "@babel/plugin-transform-runtime": "^7.10.0", | ||
| "@babel/preset-env": "^7.10.0", | ||
| "@theia/application-package": "1.69.0", | ||
| "@theia/plugin-ext": "1.69.0", | ||
| "@theia/plugin-ext-vscode": "1.69.0", |
There was a problem hiding this comment.
Major architectural concern. @theia/application-manager is a low-level dev-package that previously only depended on other dev packages. Adding @theia/core, @theia/plugin-ext, and @theia/plugin-ext-vscode as dependencies pulls in the entire runtime dependency tree into the build tool.
Conceptually this is not clean. I understand why this was done, to bring in the plugin detection logic from runtime to build time, but this needs to be done more cleanly. For example by extracting the logic into an own package which is then reused.
| /** Frontend (browser / WebWorker) plugin host id */ | ||
| export const PLUGIN_HOST_FRONTEND = 'frontend'; | ||
| /** Identifier for where a plugin runs (RPC/messaging) */ | ||
| export type PluginHost = typeof PLUGIN_HOST_FRONTEND | typeof PLUGIN_HOST_BACKEND; |
There was a problem hiding this comment.
The PluginHost type was previously 'frontend' | string (in hosted/common/hosted-plugin.ts, link). Now it's narrowed to 'frontend' | 'main'.
This is not sufficient as we also support headless plugins. So it should be at minimum frontend, main, headless. In theory we could even support more hosts, so I think having an explicit string would make sense. main is also not an ideal word, it should likely be expressing that it's tied to the frontend.
| export class PluginScannerResolverImpl implements PluginScannerResolver { | ||
| private readonly scannersByType = new Map<string, PluginScanner>(); | ||
|
|
||
| constructor(@multiInject(PluginScanner) scanners: PluginScanner[]) { |
There was a problem hiding this comment.
We don't use multiinject in Theia, we use Theia's utility ContributionProvider.
Usually we also avoid constructor injection and use field injection instead.
| private loadPromise: Promise<void> | undefined; | ||
|
|
||
| constructor( | ||
| @inject(BrowserOnlyPluginsProvider) protected readonly pluginsProvider: BrowserOnlyPluginsProvider |
| function findInMap(map: Record<string, string> | undefined, key: string): string | undefined { | ||
| if (!map) {return undefined; } | ||
| if (key in map) {return map[key]; } | ||
| const found = Object.keys(map).find(k => k.toLowerCase() === key.toLowerCase()); | ||
| return found ? map[found] : undefined; | ||
| } |
There was a problem hiding this comment.
This changes localization behavior for all plugins, not just browser-only as previously we were not case insensitive.
There was a problem hiding this comment.
Does that mean that the snippets are now "slower" when shown?
| const existing = this.themeService.getTheme(id!); | ||
|
|
||
| if (existing.id === id && existing.editorTheme) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
But the ThemeService is scoped to Theia? So whenever Theia loads we load here anyway. Do we load themes multiple times by accident? What are the steps to reproduce the issue?
| const plugin: Plugin = { | ||
| pluginPath: pluginModel.entryPoint.frontend!, | ||
| pluginFolder: pluginModel.packagePath, | ||
| pluginFolder: pluginModel.packageUri, |
There was a problem hiding this comment.
The final removal of packagePath and the consistent use of packageUri must be mentioned in the breaking changes of the changelog.
| // do not start backend plugins for browser-only | ||
| if (environment.browserOnly.is()) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
We could have another fix: backend plugins could be excluded from the very beginning in the browser-only plugin list, i.e. at build time.
| const commandDef = typeof command === 'string' ? { id: command } : command; | ||
|
|
||
| if (handler && commandIsDeclaredInPackage(commandDef.id, plugin.rawModel)) { | ||
| return commandRegistry.registerHandler(commandDef.id, handler, thisArg); |
There was a problem hiding this comment.
Seems like a drive-by fix? Could also be a separate PR
What it does
Add support for VS Code web extensions in browser-only deployments (pre-installed from
/plugins/).Fixes extension listing in the About dialog.
#12852
How to test
Install plugins as usual, build and run the browser-only example, then start the application.
Tested:
Follow-ups
Extensions that rely on backend functionality may not work fully, as in browser-only mode, only the frontend plugin code is executed.
Extensions that don't have a browser target or contributions will not be copied at all.
VS Code extension localization (
vscode.l10n) works. Localization bundles (e.g.bundle.l10n.<lang>.json) are loaded at runtime. However plugin manifests are generated with the default locale for this moment, sonlsstrings from manifests will not be localized.How it works
Loading plugins in a browser-only environment requires the following:
In a normal Theia build these tasks are handled by the backend. To support browser-only deployments:
lib/frontend/hostedPlugin. Directory names are normalized to match the paths normally provided by the backend.list.jsonfile is generated with normalized manifests for all installed plugins.Apart from these changes, the plugin loading process remains unchanged.
A small refactoring was also performed:
vscodeAPI previously received the API bound toemptyPlugin, which caused issues such as broken localization. Extensions now receive the correct plugin context.Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers