Repository: nicoespeon/abracadabra (ts)

Issue:
comments get deleted
## Describe the bug

Some refactorings delete leading/trailing comments in the current block.
This breaks userscripts where they are necessary: https://wiki.greasespot.net/Metadata_Block.

## How to reproduce

```js
// some comment
const x = 1, y = 2;
```
Run the `Split multiple declarations` refactoring:
```js
const x = 1;
const y = 2;
```

## Expected behavior

Retain the comments:
```js
// some comment
const x = 1;
const y = 2;
```

## Additional information

- Version of the extension impacted: _v6.15.3_
- Also happens with `Split declaration and initialization`, `Convert ternary if/else` and probably others. All seem to be related to `path.replaceWith/replaceWithMultiple`


Pull request:
Preserve comments when replacing nodes
Fixes #705

Some refactorings deleted comments (e.g. `Split multiple declarations`) because `path.replaceWith/replaceWithMultiple` does not inherit the `comments` property added by recast. I fixed this by creating and calling `replaceWithPreservingComments/replaceWithMultiplePreservingComments` instead.

Patch:
diff --git a/src/ast/transformation.ts b/src/ast/transformation.ts
index 22c5b827..8576d0ff 100644
--- a/src/ast/transformation.ts
+++ b/src/ast/transformation.ts
@@ -249,16 +249,43 @@ export function mergeCommentsInto<T extends t.Node>(
   node: T,
   commentedNodes: t.Node[]
 ): T {
+  const comments = commentedNodes.reduce<
+    { leading?: boolean; trailing?: boolean }[]
+  >(
+    // @ts-expect-error Recast does use a `comments` attribute.
+    (memo, { comments }) => memo.concat(comments),
+    []
+  );
+  const leadingComments = comments.filter((c) => c?.leading);
+  const trailingComments = comments.filter((c) => c?.trailing);
   return {
     ...node,
-    comments: commentedNodes.reduce(
-      // @ts-expect-error Recast does use a `comments` attribute.
-      (memo, { comments }) => memo.concat(comments),
-      []
-    )
+    leadingComments,
+    trailingComments,
+    comments
   };
 }
 
+export function replaceWithPreservingComments(
+  path: NodePath,
+  replacement: t.Node | NodePath<t.Node>
+) {
+  const replacementPath = path.replaceWith(replacement);
+  if (replacementPath[0]) {
+    preserveCommentsForRecast(replacementPath[0]);
+  }
+}
+
+export function replaceWithMultiplePreservingComments(
+  path: NodePath,
+  replacements: readonly t.Node[]
+) {
+  const replacementPaths = path.replaceWithMultiple(replacements);
+  for (const replacementPath of replacementPaths) {
+    preserveCommentsForRecast(replacementPath);
+  }
+}
+
 export type RootNodePath<T = t.Node> = NodePath<T> & {
   parentPath: NodePath<t.Program>;
 };
diff --git a/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.ts b/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.ts
index 6f9601a8..6da50ece 100644
--- a/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.ts
+++ b/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.ts
@@ -44,7 +44,7 @@ function updateCode(
               path.isFunctionDeclaration()
           );
           if (parentFunction !== fnPath) return;
-          returnPath.replaceWith(t.continueStatement());
+          t.replaceWithPreservingComments(returnPath, t.continueStatement());
         }
       });
 
@@ -53,7 +53,8 @@ function updateCode(
           path.parentPath.node.loc.start
         ).line;
       }
-      path.parentPath.replaceWith(
+      t.replaceWithPreservingComments(
+        path.parentPath,
         t.forOfStatement(
           t.variableDeclaration("const", [t.variableDeclarator(item)]),
           items,
diff --git a/src/refactorings/convert-for-to-for-each/convert-for-to-for-each.ts b/src/refactorings/convert-for-to-for-each/convert-for-to-for-each.ts
index fa99af9c..c1f97426 100644
--- a/src/refactorings/convert-for-to-for-each/convert-for-to-for-each.ts
+++ b/src/refactorings/convert-for-to-for-each/convert-for-to-for-each.ts
@@ -40,7 +40,10 @@ function updateCode(
         : t.blockStatement([body]);
 
       forLoopStartLine = Position.fromAST(path.node.loc.start).line;
-      path.replaceWith(t.forEach(list, getParams(forEachBody), forEachBody));
+      t.replaceWithPreservingComments(
+        path,
+        t.forEach(list, getParams(forEachBody), forEachBody)
+      );
       path.stop();
     })
   );
diff --git a/src/refactorings/convert-if-else-to-switch/convert-if-else-to-switch.ts b/src/refactorings/convert-if-else-to-switch/convert-if-else-to-switch.ts
index 3dc16ee2..75d3c739 100644
--- a/src/refactorings/convert-if-else-to-switch/convert-if-else-to-switch.ts
+++ b/src/refactorings/convert-if-else-to-switch/convert-if-else-to-switch.ts
@@ -18,7 +18,7 @@ function updateCode(ast: t.AST, selection: Selection): t.Transformed {
   return t.transformAST(
     ast,
     createVisitor(selection, (path, convertedNode) => {
-      path.replaceWith(convertedNode);
+      t.replaceWithPreservingComments(path, convertedNode);
       path.stop();
     })
   );
diff --git a/src/refactorings/convert-if-else-to-ternary/convert-if-else-to-ternary.ts b/src/refactorings/convert-if-else-to-ternary/convert-if-else-to-ternary.ts
index cb149859..b738e8a2 100644
--- a/src/refactorings/convert-if-else-to-ternary/convert-if-else-to-ternary.ts
+++ b/src/refactorings/convert-if-else-to-ternary/convert-if-else-to-ternary.ts
@@ -18,7 +18,7 @@ function updateCode(ast: t.AST, selection: Selection): t.Transformed {
   return t.transformAST(
     ast,
     createVisitor(selection, (path, node) => {
-      path.replaceWith(node);
+      t.replaceWithPreservingComments(path, node);
       path.stop();
     })
   );
diff --git a/src/refactorings/convert-switch-to-if-else/convert-switch-to-if-else.ts b/src/refactorings/convert-switch-to-if-else/convert-switch-to-if-else.ts
index 765498f3..a1cd35f4 100644
--- a/src/refactorings/convert-switch-to-if-else/convert-switch-to-if-else.ts
+++ b/src/refactorings/convert-switch-to-if-else/convert-switch-to-if-else.ts
@@ -20,9 +20,9 @@ function updateCode(ast: t.AST, selection: Selection): t.Transformed {
     ast,
     createVisitor(selection, (path, convertedNode) => {
       if (t.isBlockStatement(convertedNode)) {
-        path.replaceWithMultiple(convertedNode.body);
+        t.replaceWithMultiplePreservingComments(path, convertedNode.body);
       } else {
-        path.replaceWith(convertedNode);
+        t.replaceWithPreservingComments(path, convertedNode);
       }
       path.stop();
     })
diff --git a/src/refactorings/convert-ternary-to-if-else/convert-ternary-to-if-else.ts b/src/refactorings/convert-ternary-to-if-else/convert-ternary-to-if-else.ts
index ff9c541f..46a79e85 100644
--- a/src/refactorings/convert-ternary-to-if-else/convert-ternary-to-if-else.ts
+++ b/src/refactorings/convert-ternary-to-if-else/convert-ternary-to-if-else.ts
@@ -30,7 +30,8 @@ function updateCode(
     createVisitor(selection, (path: t.NodePath<t.ConditionalExpression>) => {
       const { parentPath, node } = path;
       if (t.isReturnStatement(parentPath.node)) {
-        parentPath.replaceWith(
+        t.replaceWithPreservingComments(
+          parentPath,
           createIfStatement(selection, node, t.returnStatement)
         );
 
@@ -46,7 +47,8 @@ function updateCode(
         const expressionParentPath = parentPath.parentPath;
         if (!expressionParentPath) return;
 
-        expressionParentPath.replaceWith(
+        t.replaceWithPreservingComments(
+          expressionParentPath,
           createIfStatement(selection, node, (expression) =>
             createAssignment(operator, left, expression)
           )
@@ -68,7 +70,7 @@ function updateCode(
 
         // VariableDeclarator is contained in a VariableDeclaration
         // => replace parentPath's parent path
-        parentPath.parentPath.replaceWithMultiple([
+        t.replaceWithMultiplePreservingComments(parentPath.parentPath, [
           createLetDeclaration(id),
           createIfStatement(selection, node, (expression) =>
             createAssignment("=", id, expression)
@@ -79,7 +81,8 @@ function updateCode(
         return;
       }
 
-      parentPath.replaceWith(
+      t.replaceWithPreservingComments(
+        parentPath,
         createIfStatement(selection, node, t.expressionStatement)
       );
       parentPath.stop();
diff --git a/src/refactorings/convert-to-arrow-function/convert-to-arrow-function.ts b/src/refactorings/convert-to-arrow-function/convert-to-arrow-function.ts
index ceefc353..cd53e17a 100644
--- a/src/refactorings/convert-to-arrow-function/convert-to-arrow-function.ts
+++ b/src/refactorings/convert-to-arrow-function/convert-to-arrow-function.ts
@@ -47,7 +47,7 @@ function updateCode(
         return;
       }
 
-      path.replaceWith(converter.replacementNode);
+      t.replaceWithPreservingComments(path, converter.replacementNode);
       path.stop();
     })
   );
@@ -73,11 +73,7 @@ class FunctionDeclarationConverter implements Converter {
       t.toArrowFunctionExpression(this.path)
     );
 
-    const variableDeclaration = t.variableDeclaration("const", [declarator]);
-    // @ts-expect-error Recast does use a `comments` attribute.
-    variableDeclaration.comments = node.comments;
-
-    return variableDeclaration;
+    return t.variableDeclaration("const", [declarator]);
   }
 
   get hasReferenceBefore() {
diff --git a/src/refactorings/convert-to-template-literal/convert-to-template-literal.ts b/src/refactorings/convert-to-template-literal/convert-to-template-literal.ts
index 98b133d1..9e78be96 100644
--- a/src/refactorings/convert-to-template-literal/convert-to-template-literal.ts
+++ b/src/refactorings/convert-to-template-literal/convert-to-template-literal.ts
@@ -27,7 +27,7 @@ function updateCode(ast: t.AST, selection: Selection): t.Transformed {
         // Case of <MyComponent prop="test" /> => <MyComponent prop={`test`} />
         path.replaceWith(t.jsxExpressionContainer(templateLiteral));
       } else {
-        path.replaceWith(templateLiteral);
+        t.replaceWithPreservingComments(path, templateLiteral);
       }
 
       path.stop();
diff --git a/src/refactorings/split-declaration-and-initialization/split-declaration-and-initialization.ts b/src/refactorings/split-declaration-and-initialization/split-declaration-and-initialization.ts
index ba96ab1d..eda530a0 100644
--- a/src/refactorings/split-declaration-and-initialization/split-declaration-and-initialization.ts
+++ b/src/refactorings/split-declaration-and-initialization/split-declaration-and-initialization.ts
@@ -20,7 +20,7 @@ function updateCode(ast: t.AST, selection: Selection): t.Transformed {
     createVisitor(selection, (path: t.NodePath<t.VariableDeclaration>) => {
       const declarations = path.node.declarations;
       const kind = path.node.kind === "const" ? "let" : path.node.kind;
-      path.replaceWithMultiple([
+      t.replaceWithMultiplePreservingComments(path, [
         t.variableDeclaration(
           kind,
           declarations.flatMap(({ id }) => createVariableDeclarators(id))
diff --git a/src/refactorings/split-multiple-declarations/split-multiple-declarations.ts b/src/refactorings/split-multiple-declarations/split-multiple-declarations.ts
index 2d95f29d..98ef4042 100644
--- a/src/refactorings/split-multiple-declarations/split-multiple-declarations.ts
+++ b/src/refactorings/split-multiple-declarations/split-multiple-declarations.ts
@@ -25,7 +25,7 @@ function updateCode(ast: t.AST, selection: Selection): t.Transformed {
         return t.variableDeclaration(kind, [declarator]);
       });
 
-      path.replaceWithMultiple(declarations);
+      t.replaceWithMultiplePreservingComments(path, declarations);
     })
   );
 }


Test patch:
diff --git a/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.test.ts b/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.test.ts
index 1ff7b416..8fa49197 100644
--- a/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.test.ts
+++ b/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.test.ts
@@ -169,6 +169,42 @@ items.forEach((item) => {
     console.log(value);
   }
 });`
+      },
+      {
+        description: "preserves comments",
+        code: `
+// leading comment
+[cursor]items.forEach((item) => {
+  console.log(item);
+});
+// trailing comment`,
+        expected: `
+// leading comment
+for (const item of items) {
+  console.log(item);
+}
+// trailing comment`
+      },
+      {
+        description: "preserves comments for return",
+        code: `
+items.forEach((item) => {
+  if (item.type === 'foo') {
+    // leading comment
+    return;
+    // trailing comment
+  }
+  console.log(item);
+});`,
+        expected: `
+for (const item of items) {
+  if (item.type === 'foo') {
+    // leading comment
+    continue;
+    // trailing comment
+  }
+  console.log(item);
+}`
       }
     ].map(({ description, code, expected }) => ({
       description,
diff --git a/src/refactorings/convert-for-to-for-each/convert-for-to-for-each.test.ts b/src/refactorings/convert-for-to-for-each/convert-for-to-for-each.test.ts
index de6d445e..0e12e000 100644
--- a/src/refactorings/convert-for-to-for-each/convert-for-to-for-each.test.ts
+++ b/src/refactorings/convert-for-to-for-each/convert-for-to-for-each.test.ts
@@ -274,6 +274,19 @@ items.forEach(item => {
         expected: `foo.bar.forEach(item => {
   console.log(item);
 });`
+      },
+      {
+        description: "preserves comments",
+        code: `// leading comment
+[cursor]for (let i = 0; i < items.length; i++) {
+  console.log(items[i]);
+}
+// trailing comment`,
+        expected: `// leading comment
+items.forEach(item => {
+  console.log(item);
+});
+// trailing comment`
       }
     ],
     async ({ code, expected }) => {
diff --git a/src/refactorings/convert-if-else-to-switch/convert-if-else-to-switch.test.ts b/src/refactorings/convert-if-else-to-switch/convert-if-else-to-switch.test.ts
index 37a03a4f..e57793a5 100644
--- a/src/refactorings/convert-if-else-to-switch/convert-if-else-to-switch.test.ts
+++ b/src/refactorings/convert-if-else-to-switch/convert-if-else-to-switch.test.ts
@@ -222,6 +222,31 @@ case "John":
 default:
   return sayHello();
 }`
+      },
+      {
+        description: "preserves comments",
+        code: `// leading comment
+[cursor]if (name === "Jane") {
+  sayHelloToJane();
+} else if (name === "John") {
+  sayHelloToJohn();
+} else {
+  sayHello();
+}
+// trailing comment`,
+        expected: `// leading comment
+switch (name) {
+case "Jane":
+  sayHelloToJane();
+  break;
+case "John":
+  sayHelloToJohn();
+  break;
+default:
+  sayHello();
+  break;
+}
+// trailing comment`
       }
     ],
     async ({ code, expected }) => {
diff --git a/src/refactorings/convert-if-else-to-ternary/convert-if-else-to-ternary.test.ts b/src/refactorings/convert-if-else-to-ternary/convert-if-else-to-ternary.test.ts
index 7fe928e2..2c3e8654 100644
--- a/src/refactorings/convert-if-else-to-ternary/convert-if-else-to-ternary.test.ts
+++ b/src/refactorings/convert-if-else-to-ternary/convert-if-else-to-ternary.test.ts
@@ -110,6 +110,23 @@ describe("Convert If/Else to Ternary", () => {
 }`,
         expected: `function reservationMode(daysInAdvance) {
   return daysInAdvance > 10 ? "early" : "normal";
+}`
+      },
+      {
+        description: "preserve comments before and after condition",
+        code: `function reservationMode(daysInAdvance) {
+  // leading comment
+  if ([cursor]daysInAdvance > 10) {
+    return "early";
+  } else {
+    return "normal";
+  }
+  // trailing comment
+}`,
+        expected: `function reservationMode(daysInAdvance) {
+  // leading comment
+  return daysInAdvance > 10 ? "early" : "normal";
+  // trailing comment
 }`
       },
       {
diff --git a/src/refactorings/convert-switch-to-if-else/convert-switch-to-if-else.test.ts b/src/refactorings/convert-switch-to-if-else/convert-switch-to-if-else.test.ts
index ea2e68fa..9a97c0bd 100644
--- a/src/refactorings/convert-switch-to-if-else/convert-switch-to-if-else.test.ts
+++ b/src/refactorings/convert-switch-to-if-else/convert-switch-to-if-else.test.ts
@@ -206,6 +206,40 @@ return sayHello();`
         expected: `if (tempScore === 12) {
   score += scores[tempScore];
 }`
+      },
+      {
+        description: "preserves comments",
+        code: `// leading comment
+[cursor]switch (name) {
+case "Jane":
+  sayHelloToJane();
+  break;
+default:
+  sayHello();
+  break;
+}
+// trailing comment`,
+        expected: `// leading comment
+if (name === "Jane") {
+  sayHelloToJane();
+} else {
+  sayHello();
+}
+// trailing comment`
+      },
+      {
+        description:
+          "preserves comments if switch only contains default statement",
+        code: `// leading comment
+[cursor]switch (tempScore) {
+  default:
+    score += scores[tempScore];
+    break;
+}
+// trailing comment`,
+        expected: `// leading comment
+score += scores[tempScore];
+// trailing comment`
       }
     ],
     async ({ code, expected }) => {
diff --git a/src/refactorings/convert-ternary-to-if-else/convert-ternary-to-if-else.test.ts b/src/refactorings/convert-ternary-to-if-else/convert-ternary-to-if-else.test.ts
index 65e761b5..4bd21479 100644
--- a/src/refactorings/convert-ternary-to-if-else/convert-ternary-to-if-else.test.ts
+++ b/src/refactorings/convert-ternary-to-if-else/convert-ternary-to-if-else.test.ts
@@ -19,6 +19,23 @@ describe("Convert Ternary to If/Else", () => {
   } else {
     return "normal";
   }
+}`
+      },
+      {
+        description: "preserves comments for return",
+        code: `function reservationMode(daysInAdvance) {
+  // leading comment
+  return daysInA[cursor]dvance > 10 ? "early" : "normal";
+  // trailing comment
+}`,
+        expected: `function reservationMode(daysInAdvance) {
+  // leading comment
+  if (daysInAdvance > 10) {
+    return "early";
+  } else {
+    return "normal";
+  }
+  // trailing comment
 }`
       },
       {
@@ -57,6 +74,29 @@ describe("Convert Ternary to If/Else", () => {
   }
 
   return total;
+}`
+      },
+      {
+        description: "preserves comments for assignment expression",
+        code: `function reservationMode(daysInAdvance) {
+  let mode;
+  // leading comment
+  mode = daysInA[cursor]dvance > 10 ? "early" : "normal";
+  // trailing comment
+  return mode;
+}`,
+        expected: `function reservationMode(daysInAdvance) {
+  let mode;
+
+  // leading comment
+  if (daysInAdvance > 10) {
+    mode = "early";
+  } else {
+    mode = "normal";
+  }
+
+  // trailing comment
+  return mode;
 }`
       },
       {
@@ -74,6 +114,28 @@ describe("Convert Ternary to If/Else", () => {
     mode = "normal";
   }
 
+  return mode;
+}`
+      },
+      {
+        description: "preserves comments for variable declaration",
+        code: `function reservationMode(daysInAdvance) {
+  // leading comment
+  const mode = d[cursor]aysInAdvance > 10 ? "early" : "normal";
+  // trailing comment
+  return mode;
+}`,
+        expected: `function reservationMode(daysInAdvance) {
+  // leading comment
+  let mode;
+
+  if (daysInAdvance > 10) {
+    mode = "early";
+  } else {
+    mode = "normal";
+  }
+
+  // trailing comment
   return mode;
 }`
       },
@@ -218,6 +280,23 @@ if (args.forcelink) {
   } else {
     doThat();
   }
+}`
+      },
+      {
+        description: "preserves comments for ternary that is not returned",
+        code: `function doSomething(isValid) {
+  // leading comment
+  [cursor]isValid ? doThis() : doThat();
+  // trailing comment
+}`,
+        expected: `function doSomething(isValid) {
+  // leading comment
+  if (isValid) {
+    doThis();
+  } else {
+    doThat();
+  }
+  // trailing comment
 }`
       }
     ],
diff --git a/src/refactorings/convert-to-template-literal/convert-to-template-literal.test.ts b/src/refactorings/convert-to-template-literal/convert-to-template-literal.test.ts
index 6866dde8..4b0e274a 100644
--- a/src/refactorings/convert-to-template-literal/convert-to-template-literal.test.ts
+++ b/src/refactorings/convert-to-template-literal/convert-to-template-literal.test.ts
@@ -64,6 +64,19 @@ const lastName = "Doe";`
         description: "string literal with backticks",
         code: "const a = 'Hello[cursor] `you`'",
         expected: "const a = `Hello \\`you\\``"
+      },
+      {
+        description: "preserves comments",
+        code: `const name = 
+  // leading comment
+  [cursor]"Jane"
+  // trailing comment
+  ;`,
+        expected: `const name = 
+  // leading comment
+  \`Jane\`
+  // trailing comment
+  ;`
       }
     ],
     async ({ code, expected }) => {
diff --git a/src/refactorings/split-declaration-and-initialization/split-declaration-and-initialization.test.ts b/src/refactorings/split-declaration-and-initialization/split-declaration-and-initialization.test.ts
index b2b03be1..4fcaaf83 100644
--- a/src/refactorings/split-declaration-and-initialization/split-declaration-and-initialization.test.ts
+++ b/src/refactorings/split-declaration-and-initialization/split-declaration-and-initialization.test.ts
@@ -104,6 +104,17 @@ getLastName = () => {
         code: `const { firstName, lastName, ...others } = someObject;`,
         expected: `let firstName, lastName, others;
 ({ firstName, lastName, ...others } = someObject);`
+      },
+      {
+        description: "preserves comments",
+        code: `// leading comment
+[cursor]const firstName = "Jane";
+// trailing comment`,
+        expected: `// leading comment
+let firstName;
+
+firstName = "Jane";
+// trailing comment`
       }
     ],
     async ({ code, expected }) => {
diff --git a/src/refactorings/split-multiple-declarations/split-multiple-declarations.test.ts b/src/refactorings/split-multiple-declarations/split-multiple-declarations.test.ts
index c70ab175..5d90f6a9 100644
--- a/src/refactorings/split-multiple-declarations/split-multiple-declarations.test.ts
+++ b/src/refactorings/split-multiple-declarations/split-multiple-declarations.test.ts
@@ -40,6 +40,17 @@ let details = {age: 10, country: "Moon"};`
         code: `let firstName: string = 'John', age: number = 7`,
         expected: `let firstName: string = 'John';
 let age: number = 7;`
+      },
+      {
+        description: "preserves comments",
+        code: `// leading comment
+const x = 1, y = 2;
+// trailing comment`,
+        expected: `// leading comment
+const x = 1;
+
+const y = 2;
+// trailing comment`
       }
     ],
     async ({ code, expected }) => {