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
getApplicableRefactorscan 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 callgetEditsForRefactorwith 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
strings to literals from eitherNonInteractiveRefactorNameorInteractiveRefactorNameconst enums.ApplicableRefactorInfois then split into a union of interactive and non-interactive flavors, andgetApplicableRefactorsgets an additional parameterincludeInteractivethat gets used in overloads to control the return type. Existing code, lacking theincludeInteractiveparameter, will hit an overload that returns non-interactive refactors, which can then be passed togetEditsForRefactoras usual without any additional arguments. If theincludeInteractiveparameter is included, though, the refactors returned will preventgetEditsForRefactorfrom being called without discriminating onrefactor.isInteractiveand/orrefactor.nameto ensure the correct argument is passed. (A similar system ensures consistency between this external API and internalregisterRefactorcalls.) Finally, a deprecated catch-all overload forgetEditsForRefactorhas the old signature where refactor names are plainstrings, so as not to break (but to warn) anyone who’s calling the API with a refactor name that didn’t come straight from thegetApplicableRefactorsreturn 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:
getMoveToFileRefactorInfotoo, to replacegetApplicableRefactorInfoincludeInteractiveorincludeMoveToFileparameter ontogetApplicableRefactorslike 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.