'Move to file' refactor #53542

navya9singh · 2023-03-27T16:21:57Z

…ask29988

typescript-bot · 2023-03-27T16:22:01Z

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

mjbvz

The protocol parts of this change look good to me. Excited to start testing this in VS Code

mjbvz · 2023-03-28T20:55:56Z

src/server/protocol.ts

+    /* The 'name' property from the refactoring that offered this action */
+    refactor: string;
+    /* The 'name' property from the refactoring action */
+    action: string;


Are the refactor and action properties needed? Or can we assume they are Move to file since we're making this call

Yes, I believe the refactor and the action properties are needed to compute the associated code action. getEditsForAction in types.ts matches the action property to the correct refactor action.

For this new API though, won't the action always be Move to file?

I said today that my API questions didn’t affect the protocol, but I was kind of wrong; there is a parallel. My suggestion on the LS side was to remove the move-to-file-specific LS method and allow an extra argument to be passed to the existing getEditsForRefactor method. The exact same possibility exists here: instead of making a new command, we could reuse the existing GetEditsForRefactor command if we modify its request type to allow an argument containing the targetFile property. @mjbvz thoughts?

I'm good with that. For the protocol, I'll just need the expected argument's type

src/server/protocol.ts

mjbvz · 2023-03-28T21:30:38Z

Testing this out in VS Code, a few small issues I noticed so far:

The standard library d.ts types seem to be included in the suggested file list
The current file is also included in the suggested file list

Fixes microsoft#176705 For microsoft/TypeScript#53542

mjbvz · 2023-03-28T22:55:18Z

@navya9singh microsoft/vscode#178535 Hooks up initial support for this refactoring on the VS Code side. I'm hoping to land it in the first VS code 1.78 insiders that rolls out later this week. Should make testing easier. Please also send me any feedback on the UI

Add initial support for move to file Fixes #176705 For microsoft/TypeScript#53542

andrewbranch · 2023-03-29T22:35:08Z

It looks like quite a bit of code is copied and pasted from moveToNewFile.ts. We’ll want to find a way to reuse that instead of duplicating it. Since that could be a big change, I’ll wait until that’s done to do a more detailed review.

navya9singh · 2023-03-29T23:23:26Z

It looks like quite a bit of code is copied and pasted from moveToNewFile.ts. We’ll want to find a way to reuse that instead of duplicating it. Since that could be a big change, I’ll wait until that’s done to do a more detailed review.

@andrewbranch Can the duplicated functions be places in a file under src/services ?

…nto task29988

…ask29988

andrewbranch · 2023-04-17T18:54:40Z

src/services/services.ts

+    function getMoveToRefactoringFileSuggestions(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences = emptyOptions): { newFileName: string | undefined, files: string[] | undefined } {
+        synchronizeHostData();
+        const sourceFile = getValidSourceFile(fileName);
+        const program = getProgram();


program is already available to you without declaring this constant; see the other functions in this file as an example. This is similar to other places where I mentioned that code was unnecessarily defensive—if you find yourself with a type that says it could be undefined, but your function can’t do anything useful if that object is undefined, it’s worth figuring out whether it can actually be undefined in reality. Often, the answer is that if you have an undefined, some core invariant has already been violated, and it’s better to throw than silently do nothing. Presenting the user with an empty list of files isn’t going to help the user—they’re better off seeing VS Code present them an error message saying that something went wrong. And when that user files a bug, you’ll be very happy that they were able to paste a stack trace to exactly where something went wrong instead of just saying “I tried to move this code to another file and nothing happened 🤷”

andrewbranch · 2023-04-17T18:56:17Z

src/services/services.ts

+        const allFiles = program?.getSourceFiles().filter(sourceFile => !program?.isSourceFileFromExternalLibrary(sourceFile)).map(f => f.fileName);
+        const extension = extensionFromPath(fileName);
+        const files: string[] = [];
+        if (allFiles) {
+            for (const file of allFiles) {
+                const fileExtension = extensionFromPath(file);
+                if (sourceFile === getValidSourceFile(file) || extension === Extension.Ts && fileExtension === Extension.Dts || extension === Extension.Dts && startsWith(getBaseFileName(file), "lib.") && fileExtension === Extension.Dts) {
+                    continue;
+                }
+                if (extension === fileExtension) {
+                    files.push(file);
+                }
+            }
+        }


This code iterates over the source files 3 times and creates and mutates a lot of intermediate arrays. It looks the filter, map, and for loop can be replaced with a single mapDefined call.

andrewbranch · 2023-04-17T18:56:49Z

src/services/services.ts

+                }
+            }
+        }
+        //creating new filename


Suggested change

//creating new filename

andrewbranch · 2023-04-17T18:57:17Z

src/services/services.ts

+        let newFileName;
+        if (program) {
+            newFileName = createNewFileName(sourceFile, program, getRefactorContext(sourceFile, positionOrRange, preferences, emptyOptions), host);
+        }


Don’t forget to simplify this after program is no longer typed as possibly undefined.

andrewbranch · 2023-04-17T18:59:46Z

src/services/types.ts

    applyCodeActionCommand(fileName: string, action: CodeActionCommand | CodeActionCommand[]): Promise<ApplyCodeActionCommandResult | ApplyCodeActionCommandResult[]>;

    getApplicableRefactors(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences | undefined, triggerReason?: RefactorTriggerReason, kind?: string): ApplicableRefactorInfo[];
+    getMoveToRefactoringFileSuggestions(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences | undefined, triggerReason?: RefactorTriggerReason, kind?: string): { newFileName: string | undefined, files: string[] | undefined };


After fixing services.ts, can these return type properties ever actually be undefined?

andrewbranch · 2023-04-19T20:15:20Z

#53915 can be incorporated now with something like this process:

Merge main
Delete getEditsForMoveToFileRefactor and the TS Server protocol equivalent
Define a new interface InteractiveRefactorArguments with targetFile property. (In the future, this can be split out into MoveToFileArguments and other refactor arguments, with InteractiveRefactorArguments becoming a union type.)
Add a new optional argument to LanguageService#getEditsForRefactor and Refactor#getEditsForAction of type InteractiveRefactorArguments (replacing the optional targetFile parameter on the latter).
Thread that parameter through services.ts and refactorProvider.ts.
In MoveToFile’s getAvailableActions, accept the new includeInteractiveActions parameter, and if it’s not true, return no actions.
In MoveToFile’s getEditsForAction, adjust to the parameter type change made in (4), and debug assert that it’s defined (with a useful error message) if you’re not already doing that.

…ask29988

andrewbranch · 2023-04-20T21:11:52Z

src/testRunner/unittests/tsserver/refactors.ts

-    const aTs: File = { path: "/Foo/a.ts", content: "const x = 0;" };
-    const bTs: File = {
-        path: "/Foo/b.ts", content: `import {} from "./bar";
-const a = 1;`};


Instead of deleting this test, you can modify it to use the TS Server command you expect VS Code to use now. (In doing so, you’ll find that you need another update to protocol.ts to allow the interactive refactor arguments.)

andrewbranch

Nice work! This was a long process; thanks for your patience over the many revisions. This will be an awesome feature 🙂

src/server/session.ts

sheetalkamat · 2023-04-21T18:18:10Z

src/server/session.ts

+        const { newFileName, files } = project.getLanguageService().getMoveToRefactoringFileSuggestions(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file));
+        return { newFileName, files };


Suggested change

const { newFileName, files } = project.getLanguageService().getMoveToRefactoringFileSuggestions(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file));

return { newFileName, files };

return project.getLanguageService().getMoveToRefactoringFileSuggestions(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file));

sheetalkamat

Apart from one nit it looks good to go

donaldpipowitch · 2023-04-21T20:44:52Z

Thank you so much for working on this ❤️

Vendicated · 2023-05-05T15:19:36Z

Very nice feature, thanks for your work!!

But this doesn't seem to behave correctly for overloads

Given a function like

function add(x: number, y: number): number;
function add(x: string, y: string): string;
function add(x: any, y: any) {
    return x + y;
}

if you right click any of them and use this refactor, it will only move that part

Code_-_Insiders_pnskO6qwK9.mp4

Edit: I discovered it works correctly if you highlight all the functions and then use the refactor on the selection. But it would probably be better and more intuitive to properly handle overloads even if you don't? It works correctly for jsdoc at least, so I would expect it to also work for overloads

The old "Move to a new file" also seems affected by this

DanielRosenwasser · 2023-05-05T17:26:01Z

Folks, please create separate issues instead of leaving them on a closed PR. Thanks!

navya9singh added 12 commits March 7, 2023 09:47

tests for moce to another file

17d1a6b

Works in moving to an existing blank file

98bfb65

new change

ea8de38

test2 almost works

393b093

Part 2 works for test2

fe30722

generating list of file with test cases

f54ece7

Test cases working as expected

9bfd52d

cleanup of tests and renaming to "move to file"

58f2c9d

Merge branch 'main' of https://github.com/microsoft/TypeScript into t…

e10b530

…ask29988

Resolving merge conflict changes

5603210

all tests pass

dffe218

duplicate code cleanup

28d8258

typescript-bot assigned navya9singh Mar 27, 2023

typescript-bot added the Author: Team label Mar 27, 2023

typescript-bot assigned mjbvz Mar 27, 2023

typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Mar 27, 2023

navya9singh requested review from DanielRosenwasser, andrewbranch and mjbvz March 27, 2023 16:22

mjbvz approved these changes Mar 27, 2023

View reviewed changes

mjbvz reviewed Mar 28, 2023

View reviewed changes

mjbvz added a commit to mjbvz/vscode that referenced this pull request Mar 28, 2023

Add initial support for move to file

f0990c1

Fixes microsoft#176705 For microsoft/TypeScript#53542

mjbvz mentioned this pull request Mar 28, 2023

Add initial prototype for JS/TS move to file refactoring microsoft/vscode#178535

Merged

deleting 'triggerReason' property

6ff1f26

mjbvz added a commit to microsoft/vscode that referenced this pull request Mar 28, 2023

Add initial prototype for JS/TS move to file refactoring (#178535)

ce00cd7

Add initial support for move to file Fixes #176705 For microsoft/TypeScript#53542

navya9singh added 3 commits April 17, 2023 11:28

Adressing pr comments

8af19ff

Merge branch 'task29988' of https://github.com/microsoft/TypeScript i…

08b9243

…nto task29988

Merge branch 'main' of https://github.com/microsoft/TypeScript into t…

6675cb7

…ask29988

andrewbranch requested changes Apr 17, 2023

View reviewed changes

navya9singh added 2 commits April 17, 2023 12:42

Fixing baselines

f4e73aa

adressing comments

38471a8

navya9singh added 2 commits April 20, 2023 10:05

Merge branch 'main' of https://github.com/microsoft/TypeScript into t…

0439ddc

…ask29988

Changes for new api protocol

9734d4d

andrewbranch reviewed Apr 20, 2023

View reviewed changes

navya9singh and others added 2 commits April 20, 2023 16:09

fixing tests

003113c

accepting baselines

28d8564

andrewbranch approved these changes Apr 21, 2023

View reviewed changes

sheetalkamat requested changes Apr 21, 2023

View reviewed changes

src/server/session.ts Outdated Show resolved Hide resolved

src/server/session.ts Outdated Show resolved Hide resolved

src/server/session.ts Outdated Show resolved Hide resolved

Simplifying fileSuggestions in session.ts

e61ad6d

sheetalkamat reviewed Apr 21, 2023

View reviewed changes

sheetalkamat approved these changes Apr 21, 2023

View reviewed changes

Fixing return statement

ea6441d

navya9singh merged commit d3bbef3 into main Apr 21, 2023

navya9singh deleted the task29988 branch April 21, 2023 18:48

navya9singh mentioned this pull request Apr 27, 2023

Adding missing parameter #54045

Merged

zardoy mentioned this pull request May 4, 2023

new Move to file refactoring should avoid name collisions #54130

Open

DanielRosenwasser mentioned this pull request May 5, 2023

"Move to file" does not carry all function overload and implementation signatures #54155

Closed

islami00 mentioned this pull request Oct 21, 2023

Move to file in VSCode does not work with plugin installed mrmckeb/typescript-plugin-css-modules#241

Closed

coderabbitai bot mentioned this pull request Feb 25, 2025

Test lehungio/docker-images#50

Open

guillaumebrunerie mentioned this pull request Sep 13, 2025

"Move to file" code action does not set the isInteractive flag #62446

Closed

1 task

microsoft locked as resolved and limited conversation to collaborators Oct 22, 2025

		const { newFileName, files } = project.getLanguageService().getMoveToRefactoringFileSuggestions(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file));
		return { newFileName, files };

	const { newFileName, files } = project.getLanguageService().getMoveToRefactoringFileSuggestions(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file));
	return { newFileName, files };
	return project.getLanguageService().getMoveToRefactoringFileSuggestions(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file));

'Move to file' refactor #53542

'Move to file' refactor #53542

Uh oh!

Conversation

navya9singh commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 27, 2023

Uh oh!

mjbvz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjbvz commented Mar 28, 2023

Uh oh!

mjbvz commented Mar 28, 2023

Uh oh!

andrewbranch commented Mar 29, 2023

Uh oh!

navya9singh commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewbranch commented Apr 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewbranch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheetalkamat left a comment

Choose a reason for hiding this comment

Uh oh!

donaldpipowitch commented Apr 21, 2023

Uh oh!

Vendicated commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielRosenwasser commented May 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

navya9singh commented Mar 27, 2023 •

edited

Loading

navya9singh commented Mar 29, 2023 •

edited

Loading

Vendicated commented May 5, 2023 •

edited

Loading