Enable TS Server plugins on web #47377

mjbvz · 2022-01-11T03:36:48Z

Fixes #47376
Fixes microsoft/vscode#140455

This PR adds support for loading TS Service plugins on web. There plugins are loaded slightly differently than on desktop:

We only load plugins that have a browser field in their package.json. This ensures we don't try activating non-web enabled plugins on web
Plugins should ship as es modules instead of as a common js module. The top level module should export a default function that activates the plugins.
Instead of using require to load the plugin, we use import(...)

src/webServer/webServer.ts

This prototype allows service plugins to be loaded on web TSServer Main changes: - Adds a new host entryPoint called `importServicePlugin` for overriding how plugins can be loaded. This may be async - Implement `importServicePlugin` for webServer - The web server plugin implementation looks for a `browser` field in the plugin's `package.json` - It then uses `import(...)` to load the plugin (the plugin source must be compiled to support being loaded as a module)

This more or less matches how node plugins expect the plugin module to be an init function

mjbvz · 2022-02-24T01:18:28Z

To test this locally:

Checkout this branch in the TS repo and build it by running nix gulp local
Checkout VS Code and run yarn
In VS Code, comment out the whole the CopyPlugin block here (not strictly required if you don't plan on changing VS Code sources but useful as it prevents VS Code from overwriting the custom TS build we will link in with the next step):

https://github.com/microsoft/vscode/blob/cde5781978134c0091d28a4f11b45b2f08412b4f/extensions/typescript-language-features/extension-browser.webpack.config.js#L62
Symlink your local build of TS into VS Code:

ln -s /path/to/typescript/built/local/tsserver.js /path/to/vscode/extensions/typescript-language-features/dist/browser/typescript/tsserver.web.js

In VS Code, run yarn watch to build the base editor
In another terminal run yarn watch-web to build the built-in extensions for web
After the build complete for the first time, In a third terminal, run ./scripts/code-web.sh to launch VS Code for web locally (it launches on http://localhost:8080 by default)

If you need to test a VS Code extension that contributes a plugin, follow these steps to get your local extension installed into the local web instance of VS Code: https://github.com/microsoft/vscode-docs/blob/vnext/api/extension-guides/web-extensions.md#test-your-web-extension-in-on-vscodedev

…mantic mode

src/webServer/webServer.ts

src/server/project.ts

src/server/types.ts

src/webServer/webServer.ts

- Use result value instead of try/catch (`ImportPluginResult`) - Add awaits - Add logging

typescript-bot · 2022-05-06T22:07:22Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

mjbvz · 2022-05-06T22:10:56Z

Thanks for the hep here @rbuckton!

I just tested with the latest changes and think things look good for us on the VS Code side. Any other big concerns for this on the TS side?

jakebailey · 2022-05-06T22:20:07Z

IIRC @RyanCavanaugh mentioned something about not leaving any evals in the TS codebase to avoid the wrath of the security people; should the eval get replaced with a throw if our intent is to just have importScripts?

We should throw instead when dynamic import is not implemented

mjbvz · 2022-05-10T19:01:46Z

Ok I've updated the PR to throw instead of falling back to eval. Also confirmed that if we load tsserverWeb before tsserver, we are able to shim dynamicImport

microsoft/TypeScript#47377 To run plugins on web, we need to shim out `dynamicImport`. This is done in a file call `tsserverWeb.js`, which is added by the linked PR

frank-dspeed · 2022-05-20T07:39:01Z

awsome work

rbuckton

I'm going through project.ts, editorServices.ts, and related files to determine what issues we'll run into by introducing asynchrony. All of the code paths currently expect things to execute synchronously, so making any of them async means there's a possibility of out of order updates or reading stale data.

src/server/project.ts

frank-dspeed · 2022-05-23T06:32:01Z

@rbuckton good catch but going async with plugins is the only way to go and then awaiting it all plugins are async by design as they can be ESM and that it can be ESM also says that it needs to be async by design no matter what else as ESM is async by design there is no way around that.

rbuckton · 2022-05-25T01:37:37Z

@mjbvz, I've pushed up an update that ensures dynamically imported plugins are processed in the correct order. It essentially splits enablePlugin into two phases for asynchronously imported plugins. The first phase performs all of the dynamic imports in the order the plugins were requested. The second phase invokes the default export of each plugin module in the original order they were requested.

Asynchronous plugins will be loaded after the first request that creates their related Project. Once all asynchronously imported plugins have been loaded, the project services will send a ProjectsUpdatedInBackgroundEvent event to the client. This means that there is a small window where commands executed against a project will be executed without any effects from plugins.

frank-dspeed · 2022-05-25T09:29:57Z

@rbuckton can we mittiagte the time where the plugins are not loaded via catching all commands that would run against a none existing plugin and then execute them in order as soon as the event that all got loaded is fired

we dedupe the commands before that?

rbuckton · 2022-05-25T19:04:31Z

@rbuckton can we mittiagte the time where the plugins are not loaded via catching all commands that would run against a none existing plugin and then execute them in order as soon as the event that all got loaded is fired

we dedupe the commands before that?

I'm not entirely sure whether that would be necessary. New command invocations would run against the project with its plugins after they're loaded. Command invocations that affect resting state (such as diagnostics, semantic highlighting) would likely re-run when they receive the ProjectsUpdatedInBackgroundEvent. It would also mean existing clients would need to be resilient to receiving unexpected updates for already-completed command invocations.

rbuckton · 2022-05-25T23:08:49Z

@sheetalkamat could you take a look at this as well?

rbuckton

Approving for the changes that @mjbvz made, but I'd like to have @sheetalkamat or someone else with familiarity with tsserver to review the changes I've made before this is merged.

sandersn

Looks good to me, although my knowledge is not that deep. @sheetalkamat should take a look too.

src/server/editorServices.ts

src/tsserverWeb/tsconfig.json

Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

sheetalkamat · 2022-06-03T16:40:36Z

src/server/types.ts

@@ -16,5 +18,6 @@ declare namespace ts.server {
        gc?(): void;
        trace?(s: string): void;
        require?(initialPath: string, moduleName: string): RequireResult;
+        importServicePlugin?(root: string, moduleName: string): Promise<ImportPluginResult>;


do we need different name here? Especially if in future we have compile time plugins that get used on compile on save etc ?
May be just import like require ?

@mjbvz and I discussed that earlier. This isn't as generic as import because there is specific logic for importing a browser plugin inside that function.

sheetalkamat · 2022-06-03T16:41:42Z

src/testRunner/unittests/tsserver/webServer.ts

@@ -1,3 +1,4 @@
+/* eslint-disable boolean-trivia */


Is this really needed and cant be fixed? If it cant be fixed can you pls not disable this for the whole file. instead wherever its needed.

The rule is flagged on every expect(...).eq(true) (or false) in the file. It felt like enough individual exceptions that it was worth disabling for the whole file, and labeling every call with .eq(/*value*/ true) seemed like overkill.

src/tsserverWeb/tsconfig.json

src/server/types.ts

src/tsserverWeb/tsconfig.json

src/server/editorServices.ts

sheetalkamat · 2022-06-03T17:28:13Z

src/server/editorServices.ts

+                const importPromise = project.beginEnablePluginAsync(pluginConfigEntry, searchPaths, pluginConfigOverrides);
+                this.pendingPluginEnablements ??= new Map();
+                let promises = this.pendingPluginEnablements.get(project);
+                if (!promises) this.pendingPluginEnablements.set(project, promises = []);


Handle this map when project closes?

I'm not sure it should matter. This map is built up during the course of a single request, and then is set to undefined as soon as the request completes processing (in enableRequestedPluginsAsync) so it won't hold a reference to the project for very long. The Promise for the plugin import actually has a longer lifetime than the key in the map, and removing the key early would just mean that we may fail to observe a rejected promise later.

src/server/project.ts

src/server/editorServices.ts

src/testRunner/unittests/tsserver/webServer.ts

rbuckton · 2022-06-04T19:42:38Z

I still have a few more PR feedback items to go through and some additional tests that @sheetalkamat mentioned. I'll put the rest of that together next week.

rbuckton · 2022-06-11T02:52:10Z

@sheetalkamat can you take another look?

src/server/editorServices.ts

src/testRunner/unittests/tsserver/webServer.ts

microsoft/TypeScript#47377 To run plugins on web, we need to shim out `dynamicImport`. This is done in a file call `tsserverWeb.js`, which is added by the linked PR

* Add patch for enabling new TS plugins on web approach microsoft/TypeScript#47377 To run plugins on web, we need to shim out `dynamicImport`. This is done in a file call `tsserverWeb.js`, which is added by the linked PR * Update for new files names

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jan 11, 2022

mjbvz commented Jan 11, 2022

View reviewed changes

src/webServer/webServer.ts Outdated Show resolved Hide resolved

mjbvz force-pushed the ts-plugins-on-web branch from a4719fb to 9695f53 Compare February 16, 2022 03:17

mjbvz mentioned this pull request Feb 16, 2022

Enable TS Server plugins on web #47376

Closed

mjbvz force-pushed the ts-plugins-on-web branch from 9695f53 to e2bbb6c Compare February 16, 2022 03:25

use default export from plugins

53d90b6

This more or less matches how node plugins expect the plugin module to be an init function

Allow configure plugin requests against any web servers in partial se…

34a83a3

…mantic mode

rbuckton requested changes Mar 7, 2022

View reviewed changes

mjbvz and others added 2 commits March 7, 2022 18:42

Addressing some comments

51680a1

- Use result value instead of try/catch (`ImportPluginResult`) - Add awaits - Add logging

add tsserverWeb to patch in dynamic import

85e0fca

mjbvz marked this pull request as ready for review May 6, 2022 22:07

typescript-bot assigned DanielRosenwasser May 6, 2022

Remove eval

f40a867

We should throw instead when dynamic import is not implemented

mjbvz changed the title ~~Prototyping TS Server plugins on web~~ Enable TS Server plugins on web May 10, 2022

mjbvz mentioned this pull request May 10, 2022

Add patch for enabling new TS plugins on web approach microsoft/vscode#149186

Merged

rbuckton reviewed May 21, 2022

View reviewed changes

src/server/project.ts Outdated Show resolved Hide resolved

src/server/project.ts Outdated Show resolved Hide resolved

rbuckton force-pushed the ts-plugins-on-web branch from f1e8d5c to 0c78b83 Compare May 25, 2022 01:55

Ensure dynamically imported plugins are loaded in the correct order

186cec9

rbuckton force-pushed the ts-plugins-on-web branch from 0c78b83 to 186cec9 Compare May 25, 2022 18:58

Add tests for async service plugin timing

39dac60

rbuckton requested a review from sheetalkamat May 25, 2022 23:08

rbuckton approved these changes Jun 1, 2022

View reviewed changes

sandersn approved these changes Jun 2, 2022

View reviewed changes

src/server/editorServices.ts Outdated Show resolved Hide resolved

src/tsserverWeb/tsconfig.json Outdated Show resolved Hide resolved

Update src/server/editorServices.ts

78e016c

Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

sheetalkamat requested changes Jun 3, 2022

View reviewed changes

Partial PR feedback

c760041

Rename tsserverWeb to dynamicImportCompat

9bc39d1

rbuckton force-pushed the ts-plugins-on-web branch from 8a3fcdb to 9bc39d1 Compare June 11, 2022 02:53

sheetalkamat requested changes Jun 13, 2022

View reviewed changes

Additional PR feedback

186f97e

sheetalkamat approved these changes Jun 14, 2022

View reviewed changes

rbuckton merged commit 3fc5f96 into microsoft:main Jun 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable TS Server plugins on web #47377

Enable TS Server plugins on web #47377

mjbvz commented Jan 11, 2022 •

edited

Loading

mjbvz commented Feb 24, 2022 •

edited

Loading

typescript-bot commented May 6, 2022

mjbvz commented May 6, 2022

jakebailey commented May 6, 2022

mjbvz commented May 10, 2022

frank-dspeed commented May 20, 2022

rbuckton left a comment

frank-dspeed commented May 23, 2022

rbuckton commented May 25, 2022 •

edited

Loading

frank-dspeed commented May 25, 2022

rbuckton commented May 25, 2022

rbuckton commented May 25, 2022

rbuckton left a comment

sandersn left a comment

sheetalkamat Jun 3, 2022

rbuckton Jun 4, 2022

sheetalkamat Jun 3, 2022

rbuckton Jun 4, 2022

sheetalkamat Jun 3, 2022

rbuckton Jun 4, 2022

rbuckton commented Jun 4, 2022

rbuckton commented Jun 11, 2022

Enable TS Server plugins on web #47377

Enable TS Server plugins on web #47377

Conversation

mjbvz commented Jan 11, 2022 • edited Loading

mjbvz commented Feb 24, 2022 • edited Loading

typescript-bot commented May 6, 2022

mjbvz commented May 6, 2022

jakebailey commented May 6, 2022

mjbvz commented May 10, 2022

frank-dspeed commented May 20, 2022

rbuckton left a comment

Choose a reason for hiding this comment

frank-dspeed commented May 23, 2022

rbuckton commented May 25, 2022 • edited Loading

frank-dspeed commented May 25, 2022

rbuckton commented May 25, 2022

rbuckton commented May 25, 2022

rbuckton left a comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

sheetalkamat Jun 3, 2022

Choose a reason for hiding this comment

rbuckton Jun 4, 2022

Choose a reason for hiding this comment

sheetalkamat Jun 3, 2022

Choose a reason for hiding this comment

rbuckton Jun 4, 2022

Choose a reason for hiding this comment

sheetalkamat Jun 3, 2022

Choose a reason for hiding this comment

rbuckton Jun 4, 2022

Choose a reason for hiding this comment

rbuckton commented Jun 4, 2022

rbuckton commented Jun 11, 2022

mjbvz commented Jan 11, 2022 •

edited

Loading

mjbvz commented Feb 24, 2022 •

edited

Loading

rbuckton commented May 25, 2022 •

edited

Loading