Skip to content

Commit 57dd722

Browse files
DanielRosenwasserDanielRosenwasser
authored
Merge pull request microsoft#38776 from a-tarasyuk/bug/29890
fix(29890): JSX: "extract to constant" generates invalid code
2 parents 99bec50 + c75af69 commit 57dd722

19 files changed

+585
-7
lines changed

‎src/services/refactors/extractSymbol.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -915,6 +915,9 @@ namespace ts.refactor.extractSymbol {
915915
if (range.facts & RangeFacts.IsAsyncFunction) {
916916
call = factory.createAwaitExpression(call);
917917
}
918+
if (isInJSXContent(node)) {
919+
call = factory.createJsxExpression(/*dotDotDotToken*/ undefined, call);
920+
}
918921

919922
if (exposedVariableDeclarations.length && !writes) {
920923
// No need to mix declarations and writes.
@@ -1118,12 +1121,16 @@ namespace ts.refactor.extractSymbol {
11181121
variableType,
11191122
initializer);
11201123

1121-
const localReference = factory.createPropertyAccessExpression(
1124+
let localReference: Expression = factory.createPropertyAccessExpression(
11221125
rangeFacts & RangeFacts.InStaticRegion
11231126
? factory.createIdentifier(scope.name!.getText()) // TODO: GH#18217
11241127
: factory.createThis(),
11251128
factory.createIdentifier(localNameText));
11261129

1130+
if (isInJSXContent(node)) {
1131+
localReference = factory.createJsxExpression(/*dotDotDotToken*/ undefined, localReference);
1132+
}
1133+
11271134
// Declare
11281135
const maxInsertionPos = node.pos;
11291136
const nodeToInsertBefore = getNodeToInsertPropertyBefore(maxInsertionPos, scope);
@@ -1194,12 +1201,6 @@ namespace ts.refactor.extractSymbol {
11941201
const renameLocation = getRenameLocation(edits, renameFilename, localNameText, /*isDeclaredBeforeUse*/ true);
11951202
return { renameFilename, renameLocation, edits };
11961203

1197-
function isInJSXContent(node: Node) {
1198-
if (!isJsxElement(node)) return false;
1199-
if (isJsxElement(node.parent)) return true;
1200-
return false;
1201-
}
1202-
12031204
function transformFunctionInitializerAndType(variableType: TypeNode | undefined, initializer: Expression): { variableType: TypeNode | undefined, initializer: Expression } {
12041205
// If no contextual type exists there is nothing to transfer to the function signature
12051206
if (variableType === undefined) return { variableType, initializer };
@@ -1953,4 +1954,8 @@ namespace ts.refactor.extractSymbol {
19531954
return false;
19541955
}
19551956
}
1957+
1958+
function isInJSXContent(node: Node) {
1959+
return (isJsxElement(node) || isJsxSelfClosingElement(node) || isJsxFragment(node)) && isJsxElement(node.parent);
1960+
}
19561961
}

‎tests/cases/fourslash/extract-const_jsxElement1.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////function Foo() {
6+
//// return (
7+
//// <div>
8+
//// /*a*/<span></span>/*b*/
9+
//// </div>
10+
//// );
11+
////}
12+
13+
goTo.file("a.tsx");
14+
goTo.select("a", "b");
15+
edit.applyRefactor({
16+
refactorName: "Extract Symbol",
17+
actionName: "constant_scope_1",
18+
actionDescription: "Extract to constant in global scope",
19+
newContent:
20+
`const /*RENAME*/newLocal = <span></span>;
21+
function Foo() {
22+
return (
23+
<div>
24+
{newLocal}
25+
</div>
26+
);
27+
}`
28+
});

‎tests/cases/fourslash/extract-const_jsxElement2.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////function Foo() {
6+
//// return (
7+
//// <div>
8+
//// /*a*/<span></span>/*b*/
9+
//// </div>
10+
//// );
11+
////}
12+
13+
goTo.file("a.tsx");
14+
goTo.select("a", "b");
15+
edit.applyRefactor({
16+
refactorName: "Extract Symbol",
17+
actionName: "constant_scope_0",
18+
actionDescription: "Extract to constant in enclosing scope",
19+
newContent:
20+
`function Foo() {
21+
const /*RENAME*/newLocal = <span></span>;
22+
return (
23+
<div>
24+
{newLocal}
25+
</div>
26+
);
27+
}`
28+
});

‎tests/cases/fourslash/extract-const_jsxElement3.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////declare var React: any;
6+
////class Foo extends React.Component<{}, {}> {
7+
//// render() {
8+
//// return (
9+
//// <div>
10+
//// /*a*/<span></span>/*b*/
11+
//// </div>
12+
//// );
13+
//// }
14+
////}
15+
16+
goTo.file("a.tsx");
17+
goTo.select("a", "b");
18+
edit.applyRefactor({
19+
refactorName: "Extract Symbol",
20+
actionName: "constant_scope_1",
21+
actionDescription: "Extract to readonly field in class 'Foo'",
22+
newContent:
23+
`declare var React: any;
24+
class Foo extends React.Component<{}, {}> {
25+
private readonly newProperty = <span></span>;
26+
27+
render() {
28+
return (
29+
<div>
30+
{this./*RENAME*/newProperty}
31+
</div>
32+
);
33+
}
34+
}`
35+
});

‎tests/cases/fourslash/extract-const_jsxFragment1.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////function Foo() {
6+
//// return (
7+
//// <div>
8+
//// /*a*/<></>/*b*/
9+
//// </div>
10+
//// );
11+
////}
12+
13+
goTo.file("a.tsx");
14+
goTo.select("a", "b");
15+
edit.applyRefactor({
16+
refactorName: "Extract Symbol",
17+
actionName: "constant_scope_1",
18+
actionDescription: "Extract to constant in global scope",
19+
newContent:
20+
`const /*RENAME*/newLocal = <></>;
21+
function Foo() {
22+
return (
23+
<div>
24+
{newLocal}
25+
</div>
26+
);
27+
}`
28+
});

‎tests/cases/fourslash/extract-const_jsxFragment2.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////function Foo() {
6+
//// return (
7+
//// <div>
8+
//// /*a*/<></>/*b*/
9+
//// </div>
10+
//// );
11+
////}
12+
13+
goTo.file("a.tsx");
14+
goTo.select("a", "b");
15+
edit.applyRefactor({
16+
refactorName: "Extract Symbol",
17+
actionName: "constant_scope_0",
18+
actionDescription: "Extract to constant in enclosing scope",
19+
newContent:
20+
`function Foo() {
21+
const /*RENAME*/newLocal = <></>;
22+
return (
23+
<div>
24+
{newLocal}
25+
</div>
26+
);
27+
}`
28+
});

‎tests/cases/fourslash/extract-const_jsxFragment3.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////declare var React: any;
6+
////class Foo extends React.Component<{}, {}> {
7+
//// render() {
8+
//// return (
9+
//// <div>
10+
//// /*a*/<></>/*b*/
11+
//// </div>
12+
//// );
13+
//// }
14+
////}
15+
16+
goTo.file("a.tsx");
17+
goTo.select("a", "b");
18+
edit.applyRefactor({
19+
refactorName: "Extract Symbol",
20+
actionName: "constant_scope_1",
21+
actionDescription: "Extract to readonly field in class 'Foo'",
22+
newContent:
23+
`declare var React: any;
24+
class Foo extends React.Component<{}, {}> {
25+
private readonly newProperty = <></>;
26+
27+
render() {
28+
return (
29+
<div>
30+
{this./*RENAME*/newProperty}
31+
</div>
32+
);
33+
}
34+
}`
35+
});

‎tests/cases/fourslash/extract-const_jsxSelfClosingElement1.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////function Foo() {
6+
//// return (
7+
//// <div>
8+
//// /*a*/<br />/*b*/
9+
//// </div>
10+
//// );
11+
////}
12+
13+
goTo.file("a.tsx");
14+
goTo.select("a", "b");
15+
edit.applyRefactor({
16+
refactorName: "Extract Symbol",
17+
actionName: "constant_scope_1",
18+
actionDescription: "Extract to constant in global scope",
19+
newContent:
20+
`const /*RENAME*/newLocal = <br />;
21+
function Foo() {
22+
return (
23+
<div>
24+
{newLocal}
25+
</div>
26+
);
27+
}`
28+
});

‎tests/cases/fourslash/extract-const_jsxSelfClosingElement2.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////function Foo() {
6+
//// return (
7+
//// <div>
8+
//// /*a*/<br />/*b*/
9+
//// </div>
10+
//// );
11+
////}
12+
13+
goTo.file("a.tsx");
14+
goTo.select("a", "b");
15+
edit.applyRefactor({
16+
refactorName: "Extract Symbol",
17+
actionName: "constant_scope_0",
18+
actionDescription: "Extract to constant in enclosing scope",
19+
newContent:
20+
`function Foo() {
21+
const /*RENAME*/newLocal = <br />;
22+
return (
23+
<div>
24+
{newLocal}
25+
</div>
26+
);
27+
}`
28+
});

‎tests/cases/fourslash/extract-const_jsxSelfClosingElement3.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////declare var React: any;
6+
////class Foo extends React.Component<{}, {}> {
7+
//// render() {
8+
//// return (
9+
//// <div>
10+
//// /*a*/<br />/*b*/
11+
//// </div>
12+
//// );
13+
//// }
14+
////}
15+
16+
goTo.file("a.tsx");
17+
goTo.select("a", "b");
18+
edit.applyRefactor({
19+
refactorName: "Extract Symbol",
20+
actionName: "constant_scope_1",
21+
actionDescription: "Extract to readonly field in class 'Foo'",
22+
newContent:
23+
`declare var React: any;
24+
class Foo extends React.Component<{}, {}> {
25+
private readonly newProperty = <br />;
26+
27+
render() {
28+
return (
29+
<div>
30+
{this./*RENAME*/newProperty}
31+
</div>
32+
);
33+
}
34+
}`
35+
});

‎tests/cases/fourslash/extract-method_jsxElement1.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////function Foo() {
6+
//// return (
7+
//// <div>
8+
//// /*a*/<span></span>/*b*/
9+
//// </div>
10+
//// );
11+
////}
12+
13+
goTo.file("a.tsx");
14+
goTo.select("a", "b");
15+
edit.applyRefactor({
16+
refactorName: "Extract Symbol",
17+
actionName: "function_scope_1",
18+
actionDescription: "Extract to function in global scope",
19+
newContent:
20+
`function Foo() {
21+
return (
22+
<div>
23+
{newFunction()}
24+
</div>
25+
);
26+
}
27+
28+
function /*RENAME*/newFunction() {
29+
return <span></span>;
30+
}
31+
`
32+
});

‎tests/cases/fourslash/extract-method_jsxElement2.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @jsx: preserve
4+
// @filename: a.tsx
5+
////function Foo() {
6+
//// return (
7+
//// <div>
8+
//// /*a*/<span></span>/*b*/
9+
//// </div>
10+
//// );
11+
////}
12+
13+
goTo.file("a.tsx");
14+
goTo.select("a", "b");
15+
edit.applyRefactor({
16+
refactorName: "Extract Symbol",
17+
actionName: "function_scope_0",
18+
actionDescription: "Extract to inner function in function 'Foo'",
19+
newContent:
20+
`function Foo() {
21+
return (
22+
<div>
23+
{newFunction()}
24+
</div>
25+
);
26+
27+
function /*RENAME*/newFunction() {
28+
return <span></span>;
29+
}
30+
}`
31+
});

0 commit comments

Comments
 (0)