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.

Sign up for GitHub

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

Jump to bottom

Allows plugin to register commands #44291

Merged
merged 8 commits into from
Jun 18, 2021
Merged

Allows plugin to register commands #44291

merged 8 commits into from
Jun 18, 2021

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented May 27, 2021

@Kingwl Kingwl marked this pull request as draft May 27, 2021 08:26
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 27, 2021
@typescript-bot
Copy link
Collaborator

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

@Kingwl Kingwl mentioned this pull request May 27, 2021
Open
@RyanCavanaugh
Copy link
Member

@sheetalkamat this seems about right to me. Thoughts?

sheetalkamat
sheetalkamat requested changes May 28, 2021
View reviewed changes
Copy link
Member

@sheetalkamat sheetalkamat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would rather prefer if enablePlugins passes session (along with project which is already passed in) to the plugin rather than having these extra methods and we having to deal with the additions etc

@Kingwl Kingwl force-pushed the allow_typescript_plugin_to_register_commands branch from b411c08 to fad1c05 Compare May 28, 2021 10:14
@Kingwl
Copy link
Contributor Author

Kingwl commented May 28, 2021

I'm not pretty sure what's different about these kind of projects.

sheetalkamat
sheetalkamat requested changes May 28, 2021
View reviewed changes
src/server/editorServices.ts Show resolved Hide resolved
src/server/editorServices.ts Outdated Show resolved Hide resolved
src/server/editorServices.ts Outdated Show resolved Hide resolved
sheetalkamat
sheetalkamat requested changes Jun 1, 2021
View reviewed changes
Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

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

Also add test for plugin where session is passed through and you call addProtocolHandler and test that functionality
Tests are in src/testRunner/unittests/tsserver/plugins.ts

src/server/editorServices.ts
@@ -783,10 +784,13 @@ namespace ts.server {
/*@internal*/
private packageJsonFilesMap: ESMap<Path, FileWatcher> | undefined;

/*@internal*/
readonly session: Session<unknown> | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I think you should wire TMessage thing here and PluginCreateInfo too,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the unknown is better:

If we add type parameter TMessage into editorService, We should also add the type parameter into Project and etc. Too many changes.

And we are not really care about the type of message in editorService. We just forward session into pluin.

And the handlers or addProtocolHandler does not have TMessage too. Thay are raw string.

Copy link
Member

Choose a reason for hiding this comment

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

If that’s the case we shouldn’t add it to project service options either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

sheetalkamat
sheetalkamat approved these changes Jun 8, 2021
View reviewed changes
Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

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

need to update tsserverlibrary.d.ts baseline but otherwise change looks good. Thank you for your patience and sorry for going back and forth on this one as it was little unclear if TypeParameter was needed or not until you made those changes.

@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 9, 2021

Never mind.

@RyanCavanaugh
Copy link
Member

@Kingwl are you OK if I un-draft and merge this?

@Kingwl
Copy link
Contributor Author

Kingwl commented Jun 16, 2021 •
edited
Loading

@Kingwl are you OK if I un-draft and merge this?

Sure. Please feel free to merge.
Sorry. I havn't noticed that this is a draft.

@RyanCavanaugh RyanCavanaugh marked this pull request as ready for review June 16, 2021 20:33
@sandersn
Copy link
Member

Just noticed this in the PR backlog. I'm going to merge it since it looks ready to go.

@sandersn sandersn merged commit 8d3125b into microsoft:main Jun 18, 2021
@Kingwl Kingwl deleted the allow_typescript_plugin_to_register_commands branch June 21, 2021 19:34
@andrewbranch andrewbranch mentioned this pull request Sep 1, 2021
Closed
@andrewbranch andrewbranch mentioned this pull request Sep 8, 2021
Closed
@andrewbranch andrewbranch mentioned this pull request Nov 12, 2021
Closed
@Kingwl Kingwl mentioned this pull request Apr 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sheetalkamat sheetalkamat sheetalkamat approved these changes

Assignees
No one assigned
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Kingwl @typescript-bot @RyanCavanaugh @sandersn @sheetalkamat