-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Add code actions to completion entry #48076
Conversation
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
/** | ||
* Additional code actions to be made to avoid errors. | ||
*/ | ||
codeActions?: CodeAction[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the server sends a code action, is the client required to apply them to avoid errors? What happens with an older client who isn't aware of the property? (I don't have any context on the reason for the change, just trying to understand if this is a breaking change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. What I have in mind is to use this property for a new completion scenario (#46590), and maybe for class member completions, too. And yes, the client is required to apply them to avoid errors (in this case, the actions are going to be auto-import of types we are using in the completion signature).
The new completion scenario is probably going to be opt-in, so if an editor is not updated for 4.7, I think it wouldn't be an issue, since it would neither detect the new codeActions
property nor the completion scenario that uses it.
For the old completion scenarios that provide codeActions
via CompletionEntryDetails
instead, nothing would change. If we wanted to update those as well, we could support both ways of providing codeActions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, new completion functionality is opt-in via a property on UserPreferences
. In cases where the new functionality is cheap and can safely be ignored, this may be superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, new completion functionality is opt-in via a property on
UserPreferences
. In cases where the new functionality is cheap and can safely be ignored, this may be superfluous.
I'm not sure I understand. What is superfluous?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant was that the default approach would be to not do any extra work or return any extra properties unless the editor opted in via UserPreferences
. Sometimes, you can use a simpler approach. For example, if the new property you want to add to the response is very inexpensive to compute (i.e. won't affect editor responsiveness) and can safely be ignored (i.e. won't affect editor correctness), then there's not much benefit to adding a new UserPreferences
property - you can just always return the new property and older editors will ignore it.
I guess this seems not worth it if we have to also support the old-style of returning code actions in |
No description provided.