Add 'move to new file' refactor · Pull Request #23726 · microsoft/TypeScript

ghost · 2018-04-26T23:19:54Z

The user may highlight declarations on the top-level of a file and move them to a new file. Imports will be updated, and declarations will be exported as necessary (if they were private in the old file but used in the new file, or vice-versa).

This doesn't work for nested declarations because those may need to close over things -- extractSymbol handles that and adds parameters as necessary. So it would take two refactorings to take something from an inner scope to a different file.

mhegazy · 2018-04-27T17:07:38Z

src/compiler/diagnosticMessages.json

        "category": "Message",
        "code": 95046
+    },
+    "Move to new file": {


Move to a new file ?

mhegazy · 2018-04-27T17:11:19Z

src/services/textChanges.ts

            });
        }

+        export function newFileChanges(oldFile: SourceFile, fileName: string, statements: ReadonlyArray<Statement>, newLineCharacter: string): FileTextChanges {


Can you check with @mjbvz and @amcasey that sending edits for non-existing files are ok on the editor sides.

mhegazy · 2018-04-28T19:01:00Z

src/services/refactors/moveToNewFile.ts

+        const range = createTextRangeFromSpan(getRefactorContextSpan(context));
+        const { statements } = file;
+
+        const startNodeIndex = findIndex(statements, s => s.end > range.pos);


so that means that if i select a nested function to move, we will move the containing function instead.. would not it be better to not make the refactoring available at this case?

See moveToNewFile_rangeInvalid.ts.

mhegazy · 2018-04-28T19:02:14Z

src/services/refactors/moveToNewFile.ts

+    ): ReadonlyArray<Statement> {
+        const checker = program.getTypeChecker();
+
+        if (!oldFile.externalModuleIndicator) {


what if this is a .js file with require calls?

also what should we do with require statements in a .js file. seems like we have enough information in the current file to know if we should create import or const x = require(...)..

mhegazy · 2018-04-28T19:07:31Z

src/services/refactors/moveToNewFile.ts

+        });
+    }
+
+    function deleteUnusedImports(sourceFile: SourceFile, importDecl: ImportDeclaration, changes: textChanges.ChangeTracker, isUnused: (name: Identifier) => boolean): void {


this looks a lot like the logic we use in fixUnusedIdentifer for module imports.. can we consolidate?

I don't think that's easy since fixUnusedIdentifier deletes a single identifier starting from the identifier, but here we have a set of symbols and want to recurse down starting from the sourcefile and delete everything in the set.

mhegazy · 2018-04-28T19:09:29Z

src/services/refactors/moveToNewFile.ts

+
+    function deleteUnusedOldImports(oldFile: SourceFile, toMove: ReadonlyArray<Statement>, changes: textChanges.ChangeTracker, toDelete: ReadonlySymbolSet, checker: TypeChecker) {
+        for (const statement of oldFile.statements) {
+            if (!contains(toMove, statement) && isImportDeclaration(statement)) {


what about ImportEqualsDeclaration?

mhegazy · 2018-04-30T16:04:04Z

src/services/refactors/moveToNewFile.ts

+        while (true) {
+            const name = combinePaths(inDirectory, moduleName + extension);
+            if (!host.fileExists(name)) return moduleName;
+            moduleName += "0";


nit, i would make that 1-based (more natural) and i would use a separator of sorts. VSCode adds a .1, Windows will add (2) to duplicate files.
we could go with the .1 or we can use _1.

mhegazy · 2018-04-30T16:07:30Z

src/services/refactors/moveToNewFile.ts

+
+    function getNewModuleName(movedSymbols: ReadonlySymbolSet): string {
+        let name: string | undefined;
+        movedSymbols.forEach(s => { if (name === undefined) name = symbolNameNoDefault(s); });


return forEach(movedSymbols, symbolNameNoDefault) || "newFile";

mhegazy · 2018-04-30T16:12:46Z

src/services/refactors/moveToNewFile.ts

+        });
+
+        const oldFileImport = makeImportIfNecessary(oldFileDefault, oldFileNamedImports, `./${removeFileExtension(getBaseFileName(oldFile.fileName))}`);
+        return [...copiedOldImports, ...(oldFileImport ? [oldFileImport] : emptyArray)];


nit, why not just push on copiedOldImports instead of cloning it.

mhegazy · 2018-05-01T00:11:42Z

src/services/refactors/moveToNewFile.ts

+                    if (isInImport(decl)) {
+                        oldImportsNeededByNewFile.add(symbol);
+                    }
+                    else if (isTopLevelDeclaration(decl) && !movedSymbols.has(symbol)) {


what if it is not in movedSymbols just case we have not visited it yet, e.g. a forward reference to a type or an interface.. or even a class, that exists later on in the selected range?

mhegazy · 2018-05-01T00:16:40Z

src/services/refactors/moveToNewFile.ts

+    function addEs6Export(d: TopLevelDeclarationStatement): TopLevelDeclarationStatement {
+        const modifiers = concatenate([createModifier(SyntaxKind.ExportKeyword)], d.modifiers);
+        switch (d.kind) {
+            case SyntaxKind.FunctionDeclaration:


also export import A = N

…ort`

mhegazy

Please check with @amcasey and @mjbvz regarding new file changes.

markusjohnsson · 2018-05-02T06:49:57Z

Fixes #13859

ghost · 2018-05-02T16:34:40Z

New commit fixes #23793

DanielRosenwasser · 2018-05-02T17:33:32Z

src/compiler/factory.ts

-        node.expression = parenthesizeExpressionForExpressionStatement(expression);
-        return node;
+        return createExpressionStatement(parenthesizeExpressionForExpressionStatement(expression));
+


Nit: extra newline.

We could consider linting for no-padding.

mhegazy · 2018-05-02T22:52:30Z

@Andy-MS we need to make this opt-in to avoid old versions of VS/VSCode form getting edits including new files.
so we need a new setting, allowNewFileEdits or something along these lines.

mhegazy · 2018-05-02T22:53:04Z

we also need to add the file to "files" in tsconfig.json if the original file was in the list.

ghost · 2018-05-04T17:20:52Z

Latest commit adds an allowTextChangesInNewFiles preference since older editors might have problems with the new behavior. CC @mjbvz

sheetalkamat · 2018-05-04T18:02:39Z

src/compiler/core.ts

    /** Works like Array.prototype.findIndex, returning `-1` if no element satisfying the predicate is found. */
-    export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean): number {
-        for (let i = 0; i < array.length; i++) {
+    export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean, startIndex = 0): number {


can you instead make it startIndex?: number and use let i = startIndex || 0 ?

mhegazy · 2018-05-04T22:55:06Z

src/services/refactors/moveToNewFile.ts

+        // If previous file was global, this is easy.
+        changes.createNewFile(oldFile, combinePaths(currentDirectory, newFileNameWithExtension), getNewStatements(oldFile, usage, changes, toMove, program, newModuleName));
+
+        addNewFileToTsconfig(program, changes, normalizePath(combinePaths(oldFile.fileName, "..", newFileNameWithExtension)));


should not that be the filename relative to the tsconfig.json path?

amcasey

Initial impressions

amcasey · 2018-05-08T01:53:08Z

src/services/refactors/moveToNewFile.ts

+    const refactorName = "Move to a new file";
+    registerRefactor(refactorName, {
+        getAvailableActions(context): ApplicableRefactorInfo[] {
+            if (getStatementsToMove(context) === undefined || !context.preferences.allowTextChangesInNewFiles) return undefined;


The second check looks cheaper. Would it make sense to do that first?

amcasey · 2018-05-08T01:55:41Z

src/services/refactors/moveToNewFile.ts

+        }
+    });
+
+    function getStatementsToMove(context: RefactorContext): ReadonlyArray<Statement> | undefined {


Could/should this logic be shared with extract function/constant?

The corresponding function there would be getRangeToExtract. That seems to have a lot of extract-symbol-specific logic in it, though. And here we should only be moving top-level statements.

amcasey · 2018-05-08T02:01:20Z

src/services/refactors/moveToNewFile.ts

+        ];
+    }
+
+    function deleteUnusedOldImports(oldFile: SourceFile, toMove: ReadonlyArray<Statement>, changes: textChanges.ChangeTracker, toDelete: ReadonlySymbolSet, checker: TypeChecker) {


I think the goal of this function is to remove the imports that were only needed for the code being moved. It seems though, like it might also remove imports that were unused to begin with. Personally, I think it seems strange to do a partial Organize Imports as part of this operation. If we are going to do so, then my preference would be to have the editor trigger (unless the server has some extra knowledge?) so that it can appear separately on the undo stack.

unusedImportsFromOldFile will be a subset of oldImportsNeededByNewFile, so we won't remove purely-unused imports.

amcasey · 2018-05-08T02:07:00Z

tests/cases/fourslash/moveToNewFile.ts

+/// <reference path='fourslash.ts' />
+
+// @Filename: /a.ts
+////import { a, b } from "./other";


[I'm sure this is clear from the product code, but I haven't read it in detail.] What happens if you extract an import statement. Is that disallowed?

What about a return statement? Can a return statement be moved to another file?

What happens if you extract an import statement

I think we should allow this, though there's a bug if the moved import is still needed in the old file. #23968

Can a return statement be moved to another file?

Only top-level statements can be moved, and 'return' at top-level is an errorany way.

ghost · 2018-05-09T21:41:18Z

@amcasey Good to go?

ghost requested a review from amcasey April 26, 2018 23:19

ghost force-pushed the moveToNewFile branch from 0b4c9ba to 8f62d5c Compare April 26, 2018 23:21

ghost mentioned this pull request Apr 26, 2018

Error with insertNodeAfter in file with no trailing newline #23728

Closed

Add 'move to new file' refactor

76871d2

ghost force-pushed the moveToNewFile branch from 8f62d5c to 76871d2 Compare April 26, 2018 23:26

mhegazy reviewed Apr 27, 2018

View reviewed changes

mhegazy reviewed Apr 28, 2018

View reviewed changes

mhegazy reviewed Apr 30, 2018

View reviewed changes

This was referenced Apr 30, 2018

Delete comma after removing binding name #23786

Closed

Assertion failure when combining insertNodeAfter with deleteNode #23793

Closed

Code review, and support commonjs

2bb0760

mhegazy reviewed May 1, 2018

View reviewed changes

Compute movedSymbols completely before using, and support `export imp…

8c46867

…ort`

mhegazy approved these changes May 1, 2018

View reviewed changes

Merge branch 'master' into moveToNewFile

7f40d09

ghost force-pushed the moveToNewFile branch from a09c2f7 to 7f40d09 Compare May 1, 2018 18:54

Fix assertion error: sort empty change before non-empty change

1884fe9

DanielRosenwasser reviewed May 2, 2018

View reviewed changes

Remove extra newline

67c61ff

mhegazy mentioned this pull request May 3, 2018

Refactor move file #6487

Closed

Add allowTextChangesInNewFiles preference

0f2b6b9

Andy Hanson added 2 commits May 4, 2018 10:28

Merge branch 'master' into moveToNewFile

d8b5218

Add the new file to 'files' in tsconfig

497343b

sheetalkamat reviewed May 4, 2018

View reviewed changes

Andy Hanson added 2 commits May 4, 2018 11:18

Avoid parameter initializer

9900c4b

Update API baselines

233640b

mhegazy reviewed May 4, 2018

View reviewed changes

amcasey reviewed May 8, 2018

View reviewed changes

Andy Hanson added 2 commits May 8, 2018 10:49

Use path relative to tsconfig.json

7051e32

Code review

886a937

mhegazy approved these changes May 8, 2018

View reviewed changes

Andy Hanson added 2 commits May 8, 2018 11:46

Merge branch 'master' into moveToNewFile

19d7309

Fix error where node in tsconfig file was missing a source file

662e93c

ghost force-pushed the moveToNewFile branch from 7b9cbfe to 662e93c Compare May 8, 2018 19:45

ghost merged commit 7271ec1 into master May 10, 2018

ghost deleted the moveToNewFile branch May 10, 2018 18:17

chialunwu mentioned this pull request May 11, 2018

TypeScript Rename File Refactoring microsoft/vscode#19439

Closed

mjbvz mentioned this pull request May 15, 2018

Add allowTextChangesInNewFiles to protocol.d.ts #24117

Closed

OliverJAsh mentioned this pull request Jun 11, 2018

"rename file" and "move to new file" refactors do not work when using files #24857

Closed

microsoft locked and limited conversation to collaborators Jul 31, 2018

This pull request was closed.

Conversation

ghost commented Apr 26, 2018 • edited by ghost 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy left a comment

Choose a reason for hiding this comment

Uh oh!

markusjohnsson commented May 2, 2018

Uh oh!

ghost commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented May 2, 2018

Uh oh!

mhegazy commented May 2, 2018

Uh oh!

ghost commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey 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!

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!

ghost commented May 9, 2018

Uh oh!

Reviewers

Assignees

ghost commented Apr 26, 2018 •

edited by ghost

Loading

mhegazy May 1, 2018 •

edited

Loading