Separate interactive from non-interactive refactors in API #53781
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#53542 introduces a refactor that requires an extra argument beyond what any existing refactor needs: the file name the user wants to move code to. VS Code implemented support for this via an interactive dialog with the user. Other TS Server clients and language service consumers will need to implement some way of obtaining this information too, and ensure they provide it when calling
getEditsForRefactor
.This breaks the existing contract that clients have with refactors, which is that anything returned from
getApplicableRefactors
can be executed right away, with no additional context. If #53542 were merged as-is, any existing clients that simply callgetApplicableRefactors
, allow the user to select one, and callgetEditsForRefactor
with the one selected, will break. At minimum, we need to avoid returning any refactors that require additional arguments by default. A secondary goal is to ensure that when clients do opt into receiving refactors that require additional handling, the API ensures they send the correct arguments for the correct refactor.This PR shows a possible method for doing both. Refactor names are migrated from opaque
string
s to literals from eitherNonInteractiveRefactorName
orInteractiveRefactorName
const enums.ApplicableRefactorInfo
is then split into a union of interactive and non-interactive flavors, andgetApplicableRefactors
gets an additional parameterincludeInteractive
that gets used in overloads to control the return type. Existing code, lacking theincludeInteractive
parameter, will hit an overload that returns non-interactive refactors, which can then be passed togetEditsForRefactor
as usual without any additional arguments. If theincludeInteractive
parameter is included, though, the refactors returned will preventgetEditsForRefactor
from being called without discriminating onrefactor.isInteractive
and/orrefactor.name
to ensure the correct argument is passed. (A similar system ensures consistency between this external API and internalregisterRefactor
calls.) Finally, a deprecated catch-all overload forgetEditsForRefactor
has the old signature where refactor names are plainstring
s, so as not to break (but to warn) anyone who’s calling the API with a refactor name that didn’t come straight from thegetApplicableRefactors
return value.This approach does come with a bit of cognitive overhead—hopefully mostly for us and less for API consumers—and would best serve a scenario where we continue to introduce more interactive refactors like Move To File in the future. Alternative approaches:
getMoveToFileRefactorInfo
too, to replacegetApplicableRefactorInfo
includeInteractive
orincludeMoveToFile
parameter ontogetApplicableRefactors
like this PR does, but do nothing to try to add type safety between that response andgetEditsForRefactor
, and say that if you’re going to passincludeInteractive
, you just have to make sure you call and pass the right stuff on your own.Note that the TS Server protocol side of this PR is somewhat incomplete. I wanted to get feedback on what direction we want to go first.