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

Support configuring TypeScript plugins #61756

Merged
merged 1 commit into from
Oct 29, 2018

Conversation

minestarks
Copy link
Member

Depends on microsoft/TypeScript#28106

  • Register a new command, '_typescript.configurePlugin', that may be invoked from other VSCode extensions that contribute TypeScript plugins to send additional configuration data.
  • The configuration is passed along using the configurePlugins tsserver command (see configurePlugins command for tsserver TypeScript#28106)
  • If tsserver restarts for any reason, the most recent configurations set by this command, if any, will be reset via another configurePlugins call.

@minestarks
Copy link
Member Author

@mjbvz for review please

Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few changes.

Let's try to get this in this week because we go into a stabilization period for VS Code 1.29 next week


public configurePlugin(pluginName: string, configuration: any, reconfigureOnRestart?: boolean): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

Scope this to TS 3.2:

if (this._apiVersion.gte(API.v320)) {
    ...
}

Not sure if it should also cause the command to fail. Do you need this information on your side?

) { }

public execute(pluginName: string, configuration: any) {
this.lazyClientHost.value.serviceClient.configurePlugin(pluginName, configuration, true /* reconfigureOnRestart */);
Copy link
Contributor

Choose a reason for hiding this comment

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

Accessing lazyClientHost.value will force the typescript server to be created which can be expensive. Let's try to defer this instead so that if the server has not been activated, this command just stores received config values until activation takes place.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. Though I'm stuck on where I would insert that code which sends the configuration command upon server start. For example, to listen to onTsServerStarted, I need access to the TypeScriptServiceClient object. Which starts the server as soon as it's constructed. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

One option: have a PluginConfigurationProvider class that stores the plugin config map. Then both the command and the service client share this object.

The class interface could be something like:

class PluginConfigurationProvider {
    set(name: string, config: any): void;
    
    readonly entries: Iterator<{ plugin: string, config: any }>;
 
    readonly onChangedConfiguration: vscode.Event<{ plugin: string, config: any }>;
} 

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, so what's a reliable way to check whether the service is activated yet so I know whether to send the command yet? Check lazyClientHost.hasValue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I'd try to avoid accessing the host entirely though and just go through the PluginConfigurationProvider object. Then the service can listen to the onChangedConfiguration to pick up changes that are broadcast by the implementation of set

@mjbvz mjbvz merged commit 2f18a14 into microsoft:master Oct 29, 2018
@mjbvz mjbvz added this to the October 2018 milestone Oct 29, 2018
mjbvz added a commit that referenced this pull request Oct 29, 2018
Follow up on #61756

Two fixes:

- Avoid allowing the `_typescript.configurePlugin` to activate the ts extension non-lazily by instead using a `PluginConfigProvider`
- Restrict configurePlugin to TS 3.1.4
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants