Skip to content

Separate interactive from non-interactive refactors in API #53781

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

Closed

Conversation

andrewbranch
Copy link
Member

#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 call getApplicableRefactors, allow the user to select one, and call getEditsForRefactor 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 strings to literals from either NonInteractiveRefactorName or InteractiveRefactorName const enums. ApplicableRefactorInfo is then split into a union of interactive and non-interactive flavors, and getApplicableRefactors gets an additional parameter includeInteractive that gets used in overloads to control the return type. Existing code, lacking the includeInteractive parameter, will hit an overload that returns non-interactive refactors, which can then be passed to getEditsForRefactor as usual without any additional arguments. If the includeInteractive parameter is included, though, the refactors returned will prevent getEditsForRefactor from being called without discriminating on refactor.isInteractive and/or refactor.name to ensure the correct argument is passed. (A similar system ensures consistency between this external API and internal registerRefactor calls.) Finally, a deprecated catch-all overload for getEditsForRefactor has the old signature where refactor names are plain strings, so as not to break (but to warn) anyone who’s calling the API with a refactor name that didn’t come straight from the getApplicableRefactors 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:

  1. Make Move-To-File-specific language service methods and TS Server commands, like 'Move to file' refactor #53542 already does, but with a getMoveToFileRefactorInfo too, to replace getApplicableRefactorInfo
  2. Slap the includeInteractive or includeMoveToFile parameter onto getApplicableRefactors like this PR does, but do nothing to try to add type safety between that response and getEditsForRefactor, and say that if you’re going to pass includeInteractive, 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.

@typescript-bot
Copy link
Collaborator

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, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary

@jakebailey
Copy link
Member

Honestly, I'm okay with any of the three methods.

Sickeningly, I'm partial to the one where we just don't try and do validation; we already kinda don't do anything but the most basic of validation for tsserver requests anyway.

I know that this same feature is already implemented over LSP in Pylance (and maybe elsewhere); how does that work over LSP? Is it similar enough for us to take an idea? (Especially if we ever implement said protocol.) Last I knew, it was using a series of commands, but maybe it's now implemented more properly?

(@bschnurr?)

@andrewbranch andrewbranch deleted the interactive-refactors branch April 19, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants