Merge pull request #38776 from a-tarasyuk/bug/29890

fix(29890): JSX: "extract to constant" generates invalid code
microsoft · Jun 29, 2020 · 57dd722 · 57dd722
2 parents 99bec50 + c75af69
commit 57dd722
Show file tree

Hide file tree

Showing 19 changed files with 585 additions and 7 deletions.
diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts
@@ -915,6 +915,9 @@ namespace ts.refactor.extractSymbol {
         if (range.facts & RangeFacts.IsAsyncFunction) {
             call = factory.createAwaitExpression(call);
         }
+        if (isInJSXContent(node)) {
+            call = factory.createJsxExpression(/*dotDotDotToken*/ undefined, call);
+        }
 
         if (exposedVariableDeclarations.length && !writes) {
             // No need to mix declarations and writes.
@@ -1118,12 +1121,16 @@ namespace ts.refactor.extractSymbol {
                 variableType,
                 initializer);
 
-            const localReference = factory.createPropertyAccessExpression(
+            let localReference: Expression = factory.createPropertyAccessExpression(
                 rangeFacts & RangeFacts.InStaticRegion
                     ? factory.createIdentifier(scope.name!.getText()) // TODO: GH#18217
                     : factory.createThis(),
                     factory.createIdentifier(localNameText));
 
+            if (isInJSXContent(node)) {
+                localReference = factory.createJsxExpression(/*dotDotDotToken*/ undefined, localReference);
+            }
+
             // Declare
             const maxInsertionPos = node.pos;
             const nodeToInsertBefore = getNodeToInsertPropertyBefore(maxInsertionPos, scope);
@@ -1194,12 +1201,6 @@ namespace ts.refactor.extractSymbol {
         const renameLocation = getRenameLocation(edits, renameFilename, localNameText, /*isDeclaredBeforeUse*/ true);
         return { renameFilename, renameLocation, edits };
 
-        function isInJSXContent(node: Node) {
-            if (!isJsxElement(node)) return false;
-            if (isJsxElement(node.parent)) return true;
-            return false;
-        }
-
         function transformFunctionInitializerAndType(variableType: TypeNode | undefined, initializer: Expression): { variableType: TypeNode | undefined, initializer: Expression } {
             // If no contextual type exists there is nothing to transfer to the function signature
             if (variableType === undefined) return { variableType, initializer };
@@ -1953,4 +1954,8 @@ namespace ts.refactor.extractSymbol {
                 return false;
         }
     }
+
+    function isInJSXContent(node: Node) {
+        return (isJsxElement(node) || isJsxSelfClosingElement(node) || isJsxFragment(node)) && isJsxElement(node.parent);
+    }
 }
diff --git a/tests/cases/fourslash/extract-const_jsxElement1.ts b/tests/cases/fourslash/extract-const_jsxElement1.ts
@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////function Foo() {
+////    return (
+////        <div>
+////            /*a*/<span></span>/*b*/
+////        </div>
+////    );
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_1",
+    actionDescription: "Extract to constant in global scope",
+    newContent:
+`const /*RENAME*/newLocal = <span></span>;
+function Foo() {
+    return (
+        <div>
+            {newLocal}
+        </div>
+    );
+}`
+});
diff --git a/tests/cases/fourslash/extract-const_jsxElement2.ts b/tests/cases/fourslash/extract-const_jsxElement2.ts
@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////function Foo() {
+////    return (
+////        <div>
+////            /*a*/<span></span>/*b*/
+////        </div>
+////    );
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_0",
+    actionDescription: "Extract to constant in enclosing scope",
+    newContent:
+`function Foo() {
+    const /*RENAME*/newLocal = <span></span>;
+    return (
+        <div>
+            {newLocal}
+        </div>
+    );
+}`
+});
diff --git a/tests/cases/fourslash/extract-const_jsxElement3.ts b/tests/cases/fourslash/extract-const_jsxElement3.ts
@@ -0,0 +1,35 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////declare var React: any;
+////class Foo extends React.Component<{}, {}> {
+////    render() {
+////        return (
+////            <div>
+////                /*a*/<span></span>/*b*/
+////            </div>
+////        );
+////    }
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_1",
+    actionDescription: "Extract to readonly field in class 'Foo'",
+    newContent:
+`declare var React: any;
+class Foo extends React.Component<{}, {}> {
+    private readonly newProperty = <span></span>;
+
+    render() {
+        return (
+            <div>
+                {this./*RENAME*/newProperty}
+            </div>
+        );
+    }
+}`
+});
diff --git a/tests/cases/fourslash/extract-const_jsxFragment1.ts b/tests/cases/fourslash/extract-const_jsxFragment1.ts
@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////function Foo() {
+////    return (
+////        <div>
+////            /*a*/<></>/*b*/
+////        </div>
+////    );
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_1",
+    actionDescription: "Extract to constant in global scope",
+    newContent:
+`const /*RENAME*/newLocal = <></>;
+function Foo() {
+    return (
+        <div>
+            {newLocal}
+        </div>
+    );
+}`
+});
diff --git a/tests/cases/fourslash/extract-const_jsxFragment2.ts b/tests/cases/fourslash/extract-const_jsxFragment2.ts
@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////function Foo() {
+////    return (
+////        <div>
+////            /*a*/<></>/*b*/
+////        </div>
+////    );
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_0",
+    actionDescription: "Extract to constant in enclosing scope",
+    newContent:
+`function Foo() {
+    const /*RENAME*/newLocal = <></>;
+    return (
+        <div>
+            {newLocal}
+        </div>
+    );
+}`
+});
diff --git a/tests/cases/fourslash/extract-const_jsxFragment3.ts b/tests/cases/fourslash/extract-const_jsxFragment3.ts
@@ -0,0 +1,35 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////declare var React: any;
+////class Foo extends React.Component<{}, {}> {
+////    render() {
+////        return (
+////            <div>
+////                /*a*/<></>/*b*/
+////            </div>
+////        );
+////    }
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_1",
+    actionDescription: "Extract to readonly field in class 'Foo'",
+    newContent:
+`declare var React: any;
+class Foo extends React.Component<{}, {}> {
+    private readonly newProperty = <></>;
+
+    render() {
+        return (
+            <div>
+                {this./*RENAME*/newProperty}
+            </div>
+        );
+    }
+}`
+});
diff --git a/tests/cases/fourslash/extract-const_jsxSelfClosingElement1.ts b/tests/cases/fourslash/extract-const_jsxSelfClosingElement1.ts
@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////function Foo() {
+////    return (
+////        <div>
+////            /*a*/<br />/*b*/
+////        </div>
+////    );
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_1",
+    actionDescription: "Extract to constant in global scope",
+    newContent:
+`const /*RENAME*/newLocal = <br />;
+function Foo() {
+    return (
+        <div>
+            {newLocal}
+        </div>
+    );
+}`
+});
diff --git a/tests/cases/fourslash/extract-const_jsxSelfClosingElement2.ts b/tests/cases/fourslash/extract-const_jsxSelfClosingElement2.ts
@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////function Foo() {
+////    return (
+////        <div>
+////            /*a*/<br />/*b*/
+////        </div>
+////    );
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_0",
+    actionDescription: "Extract to constant in enclosing scope",
+    newContent:
+`function Foo() {
+    const /*RENAME*/newLocal = <br />;
+    return (
+        <div>
+            {newLocal}
+        </div>
+    );
+}`
+});
diff --git a/tests/cases/fourslash/extract-const_jsxSelfClosingElement3.ts b/tests/cases/fourslash/extract-const_jsxSelfClosingElement3.ts
@@ -0,0 +1,35 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////declare var React: any;
+////class Foo extends React.Component<{}, {}> {
+////    render() {
+////        return (
+////            <div>
+////                /*a*/<br />/*b*/
+////            </div>
+////        );
+////    }
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "constant_scope_1",
+    actionDescription: "Extract to readonly field in class 'Foo'",
+    newContent:
+`declare var React: any;
+class Foo extends React.Component<{}, {}> {
+    private readonly newProperty = <br />;
+
+    render() {
+        return (
+            <div>
+                {this./*RENAME*/newProperty}
+            </div>
+        );
+    }
+}`
+});
diff --git a/tests/cases/fourslash/extract-method_jsxElement1.ts b/tests/cases/fourslash/extract-method_jsxElement1.ts
@@ -0,0 +1,32 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////function Foo() {
+////    return (
+////        <div>
+////            /*a*/<span></span>/*b*/
+////        </div>
+////    );
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "function_scope_1",
+    actionDescription: "Extract to function in global scope",
+    newContent:
+`function Foo() {
+    return (
+        <div>
+            {newFunction()}
+        </div>
+    );
+}
+
+function /*RENAME*/newFunction() {
+    return <span></span>;
+}
+`
+});
diff --git a/tests/cases/fourslash/extract-method_jsxElement2.ts b/tests/cases/fourslash/extract-method_jsxElement2.ts
@@ -0,0 +1,31 @@
+/// <reference path='fourslash.ts' />
+
+// @jsx: preserve
+// @filename: a.tsx
+////function Foo() {
+////    return (
+////        <div>
+////            /*a*/<span></span>/*b*/
+////        </div>
+////    );
+////}
+
+goTo.file("a.tsx");
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract Symbol",
+    actionName: "function_scope_0",
+    actionDescription: "Extract to inner function in function 'Foo'",
+    newContent:
+`function Foo() {
+    return (
+        <div>
+            {newFunction()}
+        </div>
+    );
+
+    function /*RENAME*/newFunction() {
+        return <span></span>;
+    }
+}`
+});