Repository: nicoespeon/abracadabra (ts)

Issue:
Consider whether to always use destructuring when extracting member expressions
Given a function for concatenating two arrays.

```javascript
export function concat(a1, a2) {
  const result = new Array(a1.length + a2.length)

  for (let i = 0; i < a1.length; i++) {
    result[i] = a1[i]
  }

  for (let i = 0; i < a2.length; i++) {
    result[i + a1.length] = a2[i]
  }

  return result
}
```

I'm repeating `a1.length` and `a2.length` a few times in the code, so I use "Extract variable" on both of them and select to replace all occurrences.

```javascript
export function concat(a1, a2) {
  const { length: a1Length } = a1
  const { length: a2Length } = a2
  const result = new Array(a1Length + a2Length)

  for (let i = 0; i < a1Length; i++) {
    result[i] = a1[i]
  }

  for (let i = 0; i < a2Length; i++) {
    result[i + a1Length] = a2[i]
  }

  return result
}
```

Since I'm extracting a property with the same name twice, I have to rename them. The result ends up being somewhat noisy, plus it is longer than the more "obvious" solution.

```javascript
export function concat(a1, a2) {
  const a1Length = a1.length
  const a2Length = a2.length
  const result = new Array(a1Length + a2Length)

  for (let i = 0; i < a1Length; i++) {
    result[i] = a1[i]
  }

  for (let i = 0; i < a2Length; i++) {
    result[i + a1Length] = a2[i]
  }

  return result
}
```

Perhaps it would be a good idea not to use destructuring if the extracted variable has a different name than the original property?

I suppose this might be difficult to do, since it looks like extraction is done before the new name has been decided. But YMMV.

## Additional information

- Version of the extension used: _v4.7.0_



Pull request:
[Extract Variable] Ask destructure strategy
Resolves #161

If a member expression can be destructured, it will now prompt the user to decide whether they want to destructure or preserve it. Default is for destructuring, as current behavior. But it would make it more convenient when destructuring is not needed.

![image](https://user-images.githubusercontent.com/1094774/95792215-fe96b880-0cb0-11eb-9bf5-dd4b09f584bf.png)


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8bdf1ce0..d3a54ffa 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Extracting a member expression used to destructure it by default. But sometimes, it's not convenient. Consider the following code:
+
+```ts
+console.log(a1.length + a2.length);
+```
+
+If you want to extract both `length`, you'll end up with such code:
+
+```ts
+const { length: a1Length } = a1;
+const { length: a2Length } = a2;
+console.log(a1Length + a2Length);
+```
+
+It's a bit verbose. Now, in such case, you'll be prompt whether you want to destructure or not. So you can easily extract variables into this:
+
+```ts
+const a1Length = a1.length;
+const a2Length = a2.length;
+console.log(a1Length + a2Length);
+```
+
 ### Fixed
 
 - Some string literals couldn't be correctly extracted if they matched reserved words like `return` or `class`. This is now fixed!
diff --git a/src/ast/identity.ts b/src/ast/identity.ts
index 56076a14..3f36f809 100644
--- a/src/ast/identity.ts
+++ b/src/ast/identity.ts
@@ -8,7 +8,7 @@ export {
   isVariableDeclarationIdentifier,
   isFunctionCallIdentifier,
   isJSXPartialElement,
-  isPartOfMemberExpression,
+  isPropertyOfMemberExpression,
   isArrayExpressionElement,
   areAllObjectProperties,
   isUndefinedLiteral,
@@ -49,8 +49,12 @@ function isJSXPartialElement(path: NodePath): boolean {
   return t.isJSXOpeningElement(path) || t.isJSXClosingElement(path);
 }
 
-function isPartOfMemberExpression(path: NodePath): boolean {
-  return t.isMemberExpression(path.parent) && t.isIdentifier(path);
+function isPropertyOfMemberExpression(path: NodePath): boolean {
+  return (
+    t.isMemberExpression(path.parent) &&
+    t.isIdentifier(path) &&
+    !areEquivalent(path.node, path.parent.object)
+  );
 }
 
 function isArrayExpressionElement(
diff --git a/src/refactorings/extract/extract-variable/destructure-strategy.ts b/src/refactorings/extract/extract-variable/destructure-strategy.ts
new file mode 100644
index 00000000..d4819dfa
--- /dev/null
+++ b/src/refactorings/extract/extract-variable/destructure-strategy.ts
@@ -0,0 +1,6 @@
+export { DestructureStrategy };
+
+enum DestructureStrategy {
+  Destructure,
+  Preserve
+}
diff --git a/src/refactorings/extract/extract-variable/extract-variable.ts b/src/refactorings/extract/extract-variable/extract-variable.ts
index e4a59d14..f84a614a 100644
--- a/src/refactorings/extract/extract-variable/extract-variable.ts
+++ b/src/refactorings/extract/extract-variable/extract-variable.ts
@@ -24,14 +24,19 @@ async function extractVariable(editor: Editor) {
     return;
   }
 
-  const choice = await askReplacementStrategy(otherOccurrences, editor);
-  if (choice === ReplacementStrategy.None) return;
+  const replacementStrategy = await askReplacementStrategy(
+    otherOccurrences,
+    editor
+  );
+  if (replacementStrategy === ReplacementStrategy.None) return;
 
   const extractedOccurrences =
-    choice === ReplacementStrategy.AllOccurrences
+    replacementStrategy === ReplacementStrategy.AllOccurrences
       ? [selectedOccurrence].concat(otherOccurrences)
       : [selectedOccurrence];
 
+  await selectedOccurrence.askModificationDetails(editor);
+
   await editor.readThenWrite(
     selectedOccurrence.selection,
     (extractedCode) => [
@@ -169,7 +174,7 @@ function isExtractableContext(node: t.Node): boolean {
 
 function isExtractable(path: t.NodePath): boolean {
   return (
-    !t.isPartOfMemberExpression(path) &&
+    !t.isPropertyOfMemberExpression(path) &&
     !t.isClassPropertyIdentifier(path) &&
     !t.isVariableDeclarationIdentifier(path) &&
     !t.isFunctionCallIdentifier(path) &&
diff --git a/src/refactorings/extract/extract-variable/occurrence.ts b/src/refactorings/extract/extract-variable/occurrence.ts
index 55d2b139..6af40324 100644
--- a/src/refactorings/extract/extract-variable/occurrence.ts
+++ b/src/refactorings/extract/extract-variable/occurrence.ts
@@ -1,4 +1,4 @@
-import { Code, Modification } from "../../../editor/editor";
+import { Code, Editor, Modification } from "../../../editor/editor";
 import { Selection } from "../../../editor/selection";
 import { Position } from "../../../editor/position";
 import * as t from "../../../ast";
@@ -10,6 +10,7 @@ import {
   ShorthandVariable
 } from "./variable";
 import { Parts } from "./parts";
+import { DestructureStrategy } from "./destructure-strategy";
 
 export { createOccurrence, Occurrence };
 
@@ -97,6 +98,10 @@ class Occurrence<T extends t.Node = t.Node> {
       value: t.isJSXText(this.path.node) ? `"${code}"` : code
     };
   }
+
+  askModificationDetails(_editor: Editor): Promise<void> {
+    return Promise.resolve();
+  }
 }
 
 class ShorthandOccurrence extends Occurrence<t.ObjectProperty> {
@@ -120,16 +125,43 @@ class ShorthandOccurrence extends Occurrence<t.ObjectProperty> {
 }
 
 class MemberExpressionOccurrence extends Occurrence<t.MemberExpression> {
+  private destructureStrategy = DestructureStrategy.Destructure;
+
   toVariableDeclaration(code: Code): { name: Code; value: Code } {
     if (this.path.node.computed) {
       return super.toVariableDeclaration(code);
     }
 
+    if (this.destructureStrategy === DestructureStrategy.Preserve) {
+      return super.toVariableDeclaration(code);
+    }
+
     return {
       name: `{ ${this.variable.name} }`,
-      value: t.generate(this.path.node.object)
+      value: this.parentObject
     };
   }
+
+  async askModificationDetails(editor: Editor) {
+    const choice = await editor.askUser([
+      {
+        label: `Destructure => \`const { ${this.variable.name} } = ${this.parentObject}\``,
+        value: DestructureStrategy.Destructure
+      },
+      {
+        label: `Preserve => \`const ${this.variable.name} = ${this.parentObject}.${this.variable.name}\``,
+        value: DestructureStrategy.Preserve
+      }
+    ]);
+
+    if (choice) {
+      this.destructureStrategy = choice.value;
+    }
+  }
+
+  private get parentObject(): Code {
+    return t.generate(this.path.node.object);
+  }
 }
 
 class PartialTemplateLiteralOccurrence extends Occurrence<t.TemplateLiteral> {


Test patch:
diff --git a/src/refactorings/extract/extract-variable/extract-variable.extractable-objects.test.ts b/src/refactorings/extract/extract-variable/extract-variable.extractable-objects.test.ts
index fdaf5f1f..0df7da6c 100644
--- a/src/refactorings/extract/extract-variable/extract-variable.extractable-objects.test.ts
+++ b/src/refactorings/extract/extract-variable/extract-variable.extractable-objects.test.ts
@@ -4,6 +4,7 @@ import { InMemoryEditor } from "../../../editor/adapters/in-memory-editor";
 import { testEach } from "../../../tests-helpers";
 
 import { extractVariable } from "./extract-variable";
+import { DestructureStrategy } from "./destructure-strategy";
 
 describe("Extract Variable - Objects we can extract", () => {
   testEach<{
@@ -206,4 +207,46 @@ assert.isTrue(
       }
     }
   );
+
+  it("should ask if user wants to destructure or not", async () => {
+    const code = `console.log(foo.bar.b[cursor]az)`;
+    const editor = new InMemoryEditor(code);
+    jest.spyOn(editor, "askUser");
+
+    await extractVariable(editor);
+
+    expect(editor.askUser).toBeCalledWith([
+      {
+        label: "Destructure => `const { baz } = foo.bar`",
+        value: DestructureStrategy.Destructure
+      },
+      {
+        label: "Preserve => `const baz = foo.bar.baz`",
+        value: DestructureStrategy.Preserve
+      }
+    ]);
+  });
+
+  it("should not ask if user wants to destructure if it can't be", async () => {
+    const code = `console.log([cursor]"hello")`;
+    const editor = new InMemoryEditor(code);
+    jest.spyOn(editor, "askUser");
+
+    await extractVariable(editor);
+
+    expect(editor.askUser).not.toBeCalled();
+  });
+
+  it("should preserve member expression if user says so", async () => {
+    const code = `console.log(foo.bar.b[cursor]az);`;
+    const editor = new InMemoryEditor(code);
+    jest
+      .spyOn(editor, "askUser")
+      .mockImplementation(([, preserve]) => Promise.resolve(preserve));
+
+    await extractVariable(editor);
+
+    expect(editor.code).toBe(`const baz = foo.bar.baz;
+console.log(baz);`);
+  });
 });
diff --git a/src/refactorings/extract/extract-variable/extract-variable.extractable.test.ts b/src/refactorings/extract/extract-variable/extract-variable.extractable.test.ts
index 4af82247..1240d2ea 100644
--- a/src/refactorings/extract/extract-variable/extract-variable.extractable.test.ts
+++ b/src/refactorings/extract/extract-variable/extract-variable.extractable.test.ts
@@ -153,6 +153,13 @@ console.log({
         code: `console.log(path.[cursor]node.name);`,
         expected: `const { node } = path;
 console.log(node.name);`
+      },
+      {
+        description:
+          "a valid path when cursor is on the first parent of the member expression",
+        code: `console.log([cursor]path.node.name);`,
+        expected: `const extracted = path;
+console.log(extracted.node.name);`
       },
       {
         description: