Repository: nicoespeon/abracadabra (ts)

Issue:
Make "Move to existing file" work with variable declarations
## Describe the bug

Whenever I try to move code in my project i get "I didn't find the code to move from current selection"

## How to reproduce

Clone & open https://github.com/qpwo/test-abracadabra and open it in vscode. Try to move `two` from fileA to fileB.

## Expected behavior

Should move it to fileB.

## Screenshots

![image](https://user-images.githubusercontent.com/10591373/157339699-f3b453bd-4d44-4f22-86d0-4c4b155e8164.png)

## Additional information

v6.11.0


Pull request:
"Move to Existing File" on variable declarations
Closes #543

https://user-images.githubusercontent.com/1094774/157898902-629a8490-880a-4ab5-a659-fb1fac61bf50.mp4




Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c8f89d79..ba19ca52 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 
 - Move to existing file now supports moving exported elements. It will preserve the export, so your module API won't break.
+- Move to existing file can now move variable declarations too!
 
 ## [6.11.0] - 2022-02-19 - Dos Oruguitas 🦋
 
diff --git a/src/ast/scope.ts b/src/ast/scope.ts
index 4920915a..393f3706 100644
--- a/src/ast/scope.ts
+++ b/src/ast/scope.ts
@@ -153,19 +153,19 @@ export function referencesInScope(
 }
 
 export function getReferencedImportDeclarations(
-  functionPath: NodePath<t.FunctionDeclaration>,
+  path: NodePath<t.BlockStatement | t.Expression | null | undefined>,
   programPath: NodePath<t.Program>
 ): t.ImportDeclaration[] {
   const result: t.ImportDeclaration[] = [];
 
   const importDeclarations = getImportDeclarations(programPath);
-  functionPath.get("body").traverse({
-    Identifier(path) {
-      if (!path.isReferenced()) return;
+  path.traverse({
+    Identifier(identifierPath) {
+      if (!identifierPath.isReferenced()) return;
 
       importDeclarations.forEach((declaration) => {
         const matchingSpecifier = declaration.specifiers.find(({ local }) =>
-          areEquivalent(local, path.node)
+          areEquivalent(local, identifierPath.node)
         );
 
         if (matchingSpecifier) {
@@ -211,28 +211,30 @@ export function getTypeReferencedImportDeclarations(
 }
 
 export function hasReferencesDefinedInSameScope(
-  functionPath: NodePath<t.FunctionDeclaration>,
+  path: NodePath,
+  bodyPath: NodePath<t.BlockStatement | t.Expression | null | undefined>,
+  nodesToExclude: (t.Node | null)[],
   programPath: NodePath<t.Program>
 ): boolean {
   let result = false;
 
-  const scopeReferencesNames = bindingNamesInScope(functionPath);
+  const scopeReferencesNames = bindingNamesInScope(path);
   const importDeclarations = getImportDeclarations(programPath);
   const importReferencesNames = importDeclarations
     .flatMap(({ specifiers }) => specifiers)
     .map(({ local }) => local.name);
-  const functionParamsNames = functionPath.node.params.flatMap(getNames);
+  const excludedNames = nodesToExclude.flatMap(getNames);
   const referencesDefinedInSameScope = scopeReferencesNames
     .filter((name) => !importReferencesNames.includes(name))
-    .filter((name) => !functionParamsNames.includes(name));
+    .filter((name) => !excludedNames.includes(name));
 
-  functionPath.get("body").traverse({
-    Identifier(path) {
-      if (!path.isReferenced()) return;
+  bodyPath.traverse({
+    Identifier(identifierPath) {
+      if (!identifierPath.isReferenced()) return;
 
-      if (referencesDefinedInSameScope.includes(path.node.name)) {
+      if (referencesDefinedInSameScope.includes(identifierPath.node.name)) {
         result = true;
-        path.stop();
+        identifierPath.stop();
       }
     }
   });
diff --git a/src/refactorings/move-to-existing-file/export-declaration.ts b/src/refactorings/move-to-existing-file/export-declaration.ts
index 2c7770f7..12bee604 100644
--- a/src/refactorings/move-to-existing-file/export-declaration.ts
+++ b/src/refactorings/move-to-existing-file/export-declaration.ts
@@ -1,13 +1,18 @@
 import * as t from "../../ast";
 
 export function getExportDeclaration(
-  path: t.RootNodePath
+  path: t.NodePath
 ): ExportDeclaration | null {
-  return path.isExportNamedDeclaration()
-    ? new NamedExportDeclaration(path)
-    : path.isExportDefaultDeclaration()
-    ? new DefaultExportDeclaration(path)
-    : null;
+  if (t.isRootNodePath(path)) return new NoExportDeclaration(path);
+
+  const { parentPath } = path;
+  if (!parentPath || !t.isRootNodePath(parentPath)) return null;
+
+  return parentPath.isExportNamedDeclaration()
+    ? new NamedExportDeclaration(parentPath)
+    : parentPath.isExportDefaultDeclaration()
+    ? new DefaultExportDeclaration(parentPath)
+    : new NoExportDeclaration(parentPath);
 }
 
 export interface ExportDeclaration {
@@ -15,6 +20,18 @@ export interface ExportDeclaration {
   replaceWith(id: t.Identifier): void;
 }
 
+class NoExportDeclaration implements ExportDeclaration {
+  constructor(private path: t.RootNodePath) {}
+
+  get parentPath(): t.NodePath<t.Program> {
+    return this.path.parentPath;
+  }
+
+  replaceWith() {
+    this.path.remove();
+  }
+}
+
 class NamedExportDeclaration implements ExportDeclaration {
   constructor(private path: t.RootNodePath<t.ExportNamedDeclaration>) {}
 
diff --git a/src/refactorings/move-to-existing-file/movable-node.ts b/src/refactorings/move-to-existing-file/movable-node.ts
index 75f33178..fdb34ea0 100644
--- a/src/refactorings/move-to-existing-file/movable-node.ts
+++ b/src/refactorings/move-to-existing-file/movable-node.ts
@@ -20,28 +20,33 @@ export class MovableEmptyStatement implements MovableNode {
   removeFrom() {}
 }
 
-export class MovableFunctionDeclaration implements MovableNode {
-  private _value: t.WithId<t.FunctionDeclaration>;
+export class MovableVariableDeclaration implements MovableNode {
+  private _value: t.VariableDeclaration;
   private _hasReferencesThatCantBeImported: boolean;
   private _referencedImportDeclarations: t.ImportDeclaration[];
 
   constructor(
-    private path: t.PathWithId<t.RootNodePath<t.FunctionDeclaration>>
+    path: t.NodePath<t.VariableDeclaration>,
+    private id: t.Identifier,
+    private exportDeclaration: ExportDeclaration
   ) {
     // We need to compute these in constructor because the `path` reference
     // will be removed and not accessible later.
     this._value = path.node;
+    const declarationPath = path.get("declarations")[0];
     this._hasReferencesThatCantBeImported = t.hasReferencesDefinedInSameScope(
-      path,
-      path.parentPath
+      declarationPath,
+      declarationPath.get("init"),
+      [],
+      exportDeclaration.parentPath
     );
     this._referencedImportDeclarations = t.getReferencedImportDeclarations(
-      path,
-      path.parentPath
+      declarationPath.get("init"),
+      exportDeclaration.parentPath
     );
   }
 
-  get value(): t.FunctionDeclaration {
+  get value(): t.VariableDeclaration {
     return this._value;
   }
 
@@ -67,15 +72,15 @@ export class MovableFunctionDeclaration implements MovableNode {
 
   removeFrom(relativePath: RelativePath) {
     t.addImportDeclaration(
-      this.path.parentPath,
-      this._value.id,
+      this.exportDeclaration.parentPath,
+      this.id,
       relativePath.withoutExtension
     );
-    this.path.remove();
+    this.exportDeclaration.replaceWith(this.id);
   }
 }
 
-export class ExportedMovableFunctionDeclaration implements MovableNode {
+export class MovableFunctionDeclaration implements MovableNode {
   private _value: t.WithId<t.FunctionDeclaration>;
   private _hasReferencesThatCantBeImported: boolean;
   private _referencedImportDeclarations: t.ImportDeclaration[];
@@ -89,10 +94,12 @@ export class ExportedMovableFunctionDeclaration implements MovableNode {
     this._value = path.node;
     this._hasReferencesThatCantBeImported = t.hasReferencesDefinedInSameScope(
       path,
+      path.get("body"),
+      path.node.params,
       exportDeclaration.parentPath
     );
     this._referencedImportDeclarations = t.getReferencedImportDeclarations(
-      path,
+      path.get("body"),
       exportDeclaration.parentPath
     );
   }
@@ -136,55 +143,6 @@ export class MovableTSTypeDeclaration implements MovableNode {
   private _hasReferencesThatCantBeImported: boolean;
   private _referencedImportDeclarations: t.ImportDeclaration[];
 
-  constructor(private path: t.PathWithId<t.RootNodePath<t.TypeDeclaration>>) {
-    this._value = path.node;
-    this._hasReferencesThatCantBeImported =
-      t.hasTypeReferencesDefinedInSameScope(path);
-    this._referencedImportDeclarations = t.getTypeReferencedImportDeclarations(
-      path,
-      path.parentPath
-    );
-  }
-
-  get value(): t.TypeDeclaration {
-    return this._value;
-  }
-
-  get hasReferencesThatCantBeImported(): boolean {
-    return this._hasReferencesThatCantBeImported;
-  }
-
-  declarationsToImportFrom(relativePath: RelativePath): t.ImportDeclaration[] {
-    return this._referencedImportDeclarations.map((declaration) => {
-      const importRelativePath = new RelativePath(
-        declaration.source.value
-      ).relativeTo(relativePath);
-
-      return {
-        ...declaration,
-        source: {
-          ...declaration.source,
-          value: importRelativePath.value
-        }
-      };
-    });
-  }
-
-  removeFrom(relativePath: RelativePath) {
-    t.addImportDeclaration(
-      this.path.parentPath,
-      this._value.id,
-      relativePath.withoutExtension
-    );
-    this.path.remove();
-  }
-}
-
-export class ExportedMovableTSTypeDeclaration implements MovableNode {
-  private _value: t.WithId<t.TypeDeclaration>;
-  private _hasReferencesThatCantBeImported: boolean;
-  private _referencedImportDeclarations: t.ImportDeclaration[];
-
   constructor(
     path: t.PathWithId<t.NodePath<t.TypeDeclaration>>,
     private exportedDeclaration: ExportDeclaration
diff --git a/src/refactorings/move-to-existing-file/move-to-existing-file.ts b/src/refactorings/move-to-existing-file/move-to-existing-file.ts
index 6a7e4e02..a624b5ec 100644
--- a/src/refactorings/move-to-existing-file/move-to-existing-file.ts
+++ b/src/refactorings/move-to-existing-file/move-to-existing-file.ts
@@ -6,8 +6,7 @@ import {
   MovableEmptyStatement,
   MovableFunctionDeclaration,
   MovableTSTypeDeclaration,
-  ExportedMovableFunctionDeclaration,
-  ExportedMovableTSTypeDeclaration
+  MovableVariableDeclaration
 } from "./movable-node";
 import { getExportDeclaration } from "./export-declaration";
 
@@ -121,6 +120,28 @@ export function createVisitor(
   onMatch: (path: t.NodePath, movableNode: MovableNode) => void
 ): t.Visitor {
   return {
+    VariableDeclaration(path) {
+      if (!selection.isInsidePath(path)) return;
+
+      const declarations = path.get("declarations");
+      if (declarations.length !== 1) return;
+
+      const declaration = declarations[0];
+      if (!t.isSelectablePath(declaration)) return;
+      if (!t.isIdentifier(declaration.node.id)) return;
+
+      const exportDeclaration = getExportDeclaration(path);
+      if (!exportDeclaration) return;
+
+      onMatch(
+        path,
+        new MovableVariableDeclaration(
+          path,
+          declaration.node.id,
+          exportDeclaration
+        )
+      );
+    },
     FunctionDeclaration(path) {
       if (!t.hasNodeId(path)) return;
       if (!selection.isInsidePath(path)) return;
@@ -131,64 +152,28 @@ export function createVisitor(
       const bodySelection = Selection.fromAST(body.node.loc);
       if (selection.end.isAfter(bodySelection.start)) return;
 
-      if (t.isRootNodePath(path)) {
-        onMatch(path, new MovableFunctionDeclaration(path));
-        return;
-      }
-
-      const { parentPath } = path;
-      if (!t.isRootNodePath(parentPath)) return;
+      const exportDeclaration = getExportDeclaration(path);
+      if (!exportDeclaration) return;
 
-      const exportDeclaration = getExportDeclaration(parentPath);
-      if (exportDeclaration) {
-        onMatch(
-          path,
-          new ExportedMovableFunctionDeclaration(path, exportDeclaration)
-        );
-        return;
-      }
+      onMatch(path, new MovableFunctionDeclaration(path, exportDeclaration));
     },
     TSTypeAliasDeclaration(path) {
       if (!t.hasNodeId(path)) return;
       if (!selection.isInsidePath(path)) return;
 
-      if (t.isRootNodePath(path)) {
-        onMatch(path, new MovableTSTypeDeclaration(path));
-        return;
-      }
+      const exportDeclaration = getExportDeclaration(path);
+      if (!exportDeclaration) return;
 
-      const { parentPath } = path;
-      if (!t.isRootNodePath(parentPath)) return;
-
-      const exportDeclaration = getExportDeclaration(parentPath);
-      if (exportDeclaration) {
-        onMatch(
-          path,
-          new ExportedMovableTSTypeDeclaration(path, exportDeclaration)
-        );
-        return;
-      }
+      onMatch(path, new MovableTSTypeDeclaration(path, exportDeclaration));
     },
     TSInterfaceDeclaration(path) {
       if (!t.hasNodeId(path)) return;
       if (!selection.isInsidePath(path)) return;
 
-      if (t.isRootNodePath(path)) {
-        onMatch(path, new MovableTSTypeDeclaration(path));
-        return;
-      }
-
-      const { parentPath } = path;
-      if (!t.isRootNodePath(parentPath)) return;
+      const exportDeclaration = getExportDeclaration(path);
+      if (!exportDeclaration) return;
 
-      const exportDeclaration = getExportDeclaration(parentPath);
-      if (exportDeclaration) {
-        onMatch(
-          path,
-          new ExportedMovableTSTypeDeclaration(path, exportDeclaration)
-        );
-        return;
-      }
+      onMatch(path, new MovableTSTypeDeclaration(path, exportDeclaration));
     }
   };
 }


Test patch:
diff --git a/src/refactorings/move-to-existing-file/move-to-existing-file.test.ts b/src/refactorings/move-to-existing-file/move-to-existing-file.test.ts
index 3d5d7861..6fe8ffdc 100644
--- a/src/refactorings/move-to-existing-file/move-to-existing-file.test.ts
+++ b/src/refactorings/move-to-existing-file/move-to-existing-file.test.ts
@@ -319,6 +319,116 @@ let someData: Data;`,
   value: string;
 }`
         }
+      },
+      {
+        description: "a root-level variable declaration",
+        setup: {
+          currentFile: `import { someValue } from "lib";
+
+const [cursor]level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high"
+};
+
+console.log(level.LOW);`,
+          otherFile: "",
+          path: new RelativePath("./other-file.ts")
+        },
+        expected: {
+          currentFile: `import { level } from "./other-file";
+import { someValue } from "lib";
+
+console.log(level.LOW);`,
+          otherFile: `export const level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high"
+};`
+        }
+      },
+      {
+        description: "a root-level variable declaration (let)",
+        setup: {
+          currentFile: `import { someValue } from "lib";
+
+let [cursor]level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high"
+};
+
+console.log(level.LOW);`,
+          otherFile: "",
+          path: new RelativePath("./other-file.ts")
+        },
+        expected: {
+          currentFile: `import { level } from "./other-file";
+import { someValue } from "lib";
+
+console.log(level.LOW);`,
+          otherFile: `export let level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high"
+};`
+        }
+      },
+      {
+        description: "a root-level variable declaration with bindings",
+        setup: {
+          currentFile: `import { otherLevels } from "../lib";
+
+const [cursor]level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high",
+  ...otherLevels
+};
+
+console.log(level.LOW);`,
+          otherFile: "",
+          path: new RelativePath("./other-file.ts")
+        },
+        expected: {
+          currentFile: `import { level } from "./other-file";
+import { otherLevels } from "../lib";
+
+console.log(level.LOW);`,
+          otherFile: `import { otherLevels } from "../lib";
+
+export const level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high",
+  ...otherLevels
+};`
+        }
+      },
+      {
+        description: "an exported variable declaration",
+        setup: {
+          currentFile: `export const [cursor]level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high"
+};
+
+console.log(level.LOW);`,
+          otherFile: "",
+          path: new RelativePath("./other-file.ts")
+        },
+        expected: {
+          currentFile: `import { level } from "./other-file";
+export { level };
+
+console.log(level.LOW);`,
+          otherFile: `export const level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high"
+};`
+        }
       }
     ],
     async ({ setup, expected }) => {
@@ -380,6 +490,44 @@ type OtherType = string;`
 }
 
 type Value = string;`
+      },
+      {
+        description:
+          "if there are other variable declarators on the same declaration",
+        code: `let [cursor]level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high"
+}, message;
+
+console.log(level.LOW);`
+      },
+      {
+        description: "a variable declaration that's not at root-level",
+        code: `function printLevel(target) {
+  const [cursor]level = {
+    LOW: "low",
+    MEDIUM: "medium",
+    HIGH: "high"
+  };
+
+  console.log(level[target]);
+}
+
+printLevel("LOW");`
+      },
+      {
+        description: "a variable declaration with bindings in the same file",
+        code: `const otherLevels = { TEMPERATE: "temperate" };
+
+const [cursor]level = {
+  LOW: "low",
+  MEDIUM: "medium",
+  HIGH: "high",
+  ...otherLevels
+};
+
+console.log(level.LOW);`
       }
     ],
     async ({ code }) => {