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

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.

Move to a new file ?

Can you check with Matt Bierner (@mjbvz) and Andrew Casey (@amcasey) that sending edits for non-existing files are ok on the editor sides.

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.

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(...)..

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.

what about ImportEqualsDeclaration?

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.

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

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

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?

also export import A = N

…ort`

Please check with Andrew Casey (@amcasey) and Matt Bierner (@mjbvz) regarding new file changes.

Fixes #13859

New commit fixes #23793

Nit: extra newline.

We could consider linting for no-padding.

Andy (Andrewkraft) (@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.

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

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

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

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

Initial impressions

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

👍

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.

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.

[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.

Andrew Casey (@amcasey) Good to go?

ghost requested a review from Andrew Casey (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

Mohamed Hegazy (mhegazy) reviewed Apr 27, 2018

View reviewed changes

Mohamed Hegazy (mhegazy) reviewed Apr 28, 2018

View reviewed changes

Mohamed Hegazy (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

Mohamed Hegazy (mhegazy) reviewed May 1, 2018

View reviewed changes

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

8c46867

…ort`

Mohamed Hegazy (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

Daniel Rosenwasser (DanielRosenwasser) reviewed May 2, 2018

View reviewed changes

Remove extra newline

67c61ff

Mohamed Hegazy (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

Sheetal Nandi (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

Mohamed Hegazy (mhegazy) reviewed May 4, 2018

View reviewed changes

Andrew Casey (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

Mohamed Hegazy (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

Chia-Lun Wu (chialunwu) mentioned this pull request May 11, 2018

TypeScript Rename File Refactoring microsoft/vscode#19439

Closed

Matt Bierner (mjbvz) mentioned this pull request May 15, 2018

Add allowTextChangesInNewFiles to protocol.d.ts #24117

Closed

Oliver Joseph Ash (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 (microsoft) locked and limited conversation to collaborators Jul 31, 2018

This pull request was closed.

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

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

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

                   /** 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 {

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

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

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

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

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

+                  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)) {

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

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

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

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

+                      // 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)));

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

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

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

Uh oh!

Add 'move to new file' refactor - #23726

Add 'move to new file' refactor#2372615 commits merged intomastermicrosoft/TypeScript:masterfrom moveToNewFilemicrosoft/TypeScript:moveToNewFileCopy head branch name to clipboard

Conversation

Deleted user (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!

Mohamed Hegazy (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!

Mohamed Hegazy (mhegazy) left a comment

Choose a reason for hiding this comment

Uh oh!

Markus Johnsson (markusjohnsson) commented May 2, 2018

Uh oh!

Deleted user (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!

Mohamed Hegazy (mhegazy) commented May 2, 2018

Uh oh!

Mohamed Hegazy (mhegazy) commented May 2, 2018

Uh oh!

Deleted user (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!

Andrew Casey (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!

Add 'move to new file' refactor#23726
15 commits merged into
masterfrom
moveToNewFile

Deleted user (ghost) commented Apr 26, 2018 •
edited by ghost
Loading

Mohamed Hegazy (mhegazy) May 1, 2018 •
edited
Loading