From fec45771f489ec970bee92fa7772df693617886f Mon Sep 17 00:00:00 2001 From: Zach Whitesel Date: Mon, 8 Aug 2022 14:18:24 -0400 Subject: [PATCH 1/4] Nested ExpressionStatements Issue #144 - Prevents code duplication when nested nested statement expressions need are run though the `add-conversions` pluggin. - Previously add-conversions would generate two over lapping updates which `apply()` can not handle correctly. --- .../src/plugins/add-conversions.ts | 17 +++++++++++++++-- .../tests/src/add-conversions.test.ts | 7 +++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/ts-migrate-plugins/src/plugins/add-conversions.ts b/packages/ts-migrate-plugins/src/plugins/add-conversions.ts index 370948f..3574346 100644 --- a/packages/ts-migrate-plugins/src/plugins/add-conversions.ts +++ b/packages/ts-migrate-plugins/src/plugins/add-conversions.ts @@ -81,7 +81,7 @@ const addConversionsTransformerFactory = node = factory.createAsExpression(node as ts.Expression, anyType); } - if (shouldReplace(node)) { + if (shouldReplace(origNode, node)) { replaceNode(origNode, node); return origNode; } @@ -157,7 +157,11 @@ const addConversionsTransformerFactory = * There is still some risk of losing whitespace if the expression is contained within * an if statement condition or other construct that can contain blocks. */ -function shouldReplace(node: ts.Node): boolean { +function shouldReplace(origNode: ts.Node, node: ts.Node): boolean { + if (node.kind == ts.SyntaxKind.ExpressionStatement && ancestorIsExpressionStatement(origNode)) { + return false; + } + if (isStatement(node)) { return true; } @@ -174,6 +178,15 @@ function shouldReplace(node: ts.Node): boolean { } } +function ancestorIsExpressionStatement(origNode: ts.Node): boolean { + if (origNode.parent == undefined) + { + return false; + } else { + return origNode.parent.kind == ts.SyntaxKind.ExpressionStatement || ancestorIsExpressionStatement(origNode.parent) + } +} + function isStatement(node: ts.Node): node is ts.Statement { return ts.SyntaxKind.FirstStatement <= node.kind && node.kind <= ts.SyntaxKind.LastStatement; } diff --git a/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts b/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts index 54f6e62..540de43 100644 --- a/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts +++ b/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts @@ -113,4 +113,11 @@ class PublishEvent { } `); }); + + it('handles anonymous functions that require as any without duplicating lines (issue #144)', async () => { + const text = `var window = { onResetData: function () { this.clearNextPush = function () { this.setState({ history: [] }); }; } };`; + const result = addConversionsPlugin.run(await realPluginParams({ text })); + + expect(result).toBe(`var window = { onResetData: function () { (this as any).clearNextPush = function () { (this as any).setState({ history: [] }); }; } };`); + }); }); From 909f171f8a912702ba4bae905a7dac15412d8eb9 Mon Sep 17 00:00:00 2001 From: Zach Whitesel Date: Tue, 9 Aug 2022 09:49:55 -0400 Subject: [PATCH 2/4] linting changes --- .../ts-migrate-plugins/src/plugins/add-conversions.ts | 11 ++++++----- .../tests/src/add-conversions.test.ts | 4 +++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/ts-migrate-plugins/src/plugins/add-conversions.ts b/packages/ts-migrate-plugins/src/plugins/add-conversions.ts index 3574346..e4b4f22 100644 --- a/packages/ts-migrate-plugins/src/plugins/add-conversions.ts +++ b/packages/ts-migrate-plugins/src/plugins/add-conversions.ts @@ -158,7 +158,7 @@ const addConversionsTransformerFactory = * an if statement condition or other construct that can contain blocks. */ function shouldReplace(origNode: ts.Node, node: ts.Node): boolean { - if (node.kind == ts.SyntaxKind.ExpressionStatement && ancestorIsExpressionStatement(origNode)) { + if (node.kind === ts.SyntaxKind.ExpressionStatement && ancestorIsExpressionStatement(origNode)) { return false; } @@ -179,12 +179,13 @@ function shouldReplace(origNode: ts.Node, node: ts.Node): boolean { } function ancestorIsExpressionStatement(origNode: ts.Node): boolean { - if (origNode.parent == undefined) - { + if (origNode.parent === undefined) { return false; - } else { - return origNode.parent.kind == ts.SyntaxKind.ExpressionStatement || ancestorIsExpressionStatement(origNode.parent) } + return ( + origNode.parent.kind === ts.SyntaxKind.ExpressionStatement || + ancestorIsExpressionStatement(origNode.parent) + ); } function isStatement(node: ts.Node): node is ts.Statement { diff --git a/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts b/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts index 540de43..b1a16cb 100644 --- a/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts +++ b/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts @@ -118,6 +118,8 @@ class PublishEvent { const text = `var window = { onResetData: function () { this.clearNextPush = function () { this.setState({ history: [] }); }; } };`; const result = addConversionsPlugin.run(await realPluginParams({ text })); - expect(result).toBe(`var window = { onResetData: function () { (this as any).clearNextPush = function () { (this as any).setState({ history: [] }); }; } };`); + expect(result).toBe( + `var window = { onResetData: function () { (this as any).clearNextPush = function () { (this as any).setState({ history: [] }); }; } };`, + ); }); }); From 3dc94ac2f52ff888dee5ad1ec869d3c1ff92119b Mon Sep 17 00:00:00 2001 From: Zach Whitesel Date: Tue, 9 Aug 2022 15:02:41 -0400 Subject: [PATCH 3/4] Always Check for Ancestor Expression Statements - This fix handles nesting other stantments inside of a expression statement - Added additional test --- .../ts-migrate-plugins/src/plugins/add-conversions.ts | 2 +- .../tests/src/add-conversions.test.ts | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/ts-migrate-plugins/src/plugins/add-conversions.ts b/packages/ts-migrate-plugins/src/plugins/add-conversions.ts index e4b4f22..afa3263 100644 --- a/packages/ts-migrate-plugins/src/plugins/add-conversions.ts +++ b/packages/ts-migrate-plugins/src/plugins/add-conversions.ts @@ -158,7 +158,7 @@ const addConversionsTransformerFactory = * an if statement condition or other construct that can contain blocks. */ function shouldReplace(origNode: ts.Node, node: ts.Node): boolean { - if (node.kind === ts.SyntaxKind.ExpressionStatement && ancestorIsExpressionStatement(origNode)) { + if (ancestorIsExpressionStatement(origNode)) { return false; } diff --git a/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts b/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts index b1a16cb..e9ca606 100644 --- a/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts +++ b/packages/ts-migrate-plugins/tests/src/add-conversions.test.ts @@ -114,7 +114,7 @@ class PublishEvent { `); }); - it('handles anonymous functions that require as any without duplicating lines (issue #144)', async () => { + it('Nested Expression Statements (issue #144) Part 1: Expression Statement -> Expression Statement', async () => { const text = `var window = { onResetData: function () { this.clearNextPush = function () { this.setState({ history: [] }); }; } };`; const result = addConversionsPlugin.run(await realPluginParams({ text })); @@ -122,4 +122,13 @@ class PublishEvent { `var window = { onResetData: function () { (this as any).clearNextPush = function () { (this as any).setState({ history: [] }); }; } };`, ); }); + + it('Nested Expression Statements (issue #144) Part 2: Expression Statement -> If Statement -> Expression Statement', async () => { + const text = `const window = { onResetData() { this.clearNextPush = function () {\n if (this.setState) {\n this.setState({ history: [] });\n} }; } };`; + const result = addConversionsPlugin.run(await realPluginParams({ text })); + + expect(result).toBe( + `const window = { onResetData() { (this as any).clearNextPush = function () {\n if ((this as any).setState) {\n (this as any).setState({ history: [] });\n }\n}; } };`, + ); + }); }); From b0fe59a7ea9e2c9399872fa3904599cbc844a0c1 Mon Sep 17 00:00:00 2001 From: Zach Whitesel Date: Wed, 10 Aug 2022 11:41:16 -0400 Subject: [PATCH 4/4] Replace Recursive Function with Map - Use a map to keep track of which nodes are expression statments - Check the map when deciding if we should replace a node --- .../src/plugins/add-conversions.ts | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/packages/ts-migrate-plugins/src/plugins/add-conversions.ts b/packages/ts-migrate-plugins/src/plugins/add-conversions.ts index afa3263..d6481f6 100644 --- a/packages/ts-migrate-plugins/src/plugins/add-conversions.ts +++ b/packages/ts-migrate-plugins/src/plugins/add-conversions.ts @@ -43,7 +43,7 @@ const addConversionsTransformerFactory = : factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword); let nodesToConvert: Set; - + const ancestorReplaceMap = new Map(); return (file: ts.SourceFile) => { nodesToConvert = new Set( diags @@ -71,6 +71,14 @@ const addConversionsTransformerFactory = }; function visit(origNode: ts.Node): ts.Node | undefined { + const ancestorShouldBeReplaced = ancestorReplaceMap.get(origNode.parent); + ancestorReplaceMap.set( + origNode, + ancestorShouldBeReplaced === undefined + ? origNode.kind === ts.SyntaxKind.ExpressionStatement + : origNode.kind === ts.SyntaxKind.ExpressionStatement || ancestorShouldBeReplaced, + ); + const needsConversion = nodesToConvert.has(origNode); let node = ts.visitEachChild(origNode, visit, context); if (node === origNode && !needsConversion) { @@ -81,7 +89,7 @@ const addConversionsTransformerFactory = node = factory.createAsExpression(node as ts.Expression, anyType); } - if (shouldReplace(origNode, node)) { + if (shouldReplace(node) && !ancestorShouldBeReplaced) { replaceNode(origNode, node); return origNode; } @@ -157,11 +165,7 @@ const addConversionsTransformerFactory = * There is still some risk of losing whitespace if the expression is contained within * an if statement condition or other construct that can contain blocks. */ -function shouldReplace(origNode: ts.Node, node: ts.Node): boolean { - if (ancestorIsExpressionStatement(origNode)) { - return false; - } - +function shouldReplace(node: ts.Node): boolean { if (isStatement(node)) { return true; } @@ -178,16 +182,6 @@ function shouldReplace(origNode: ts.Node, node: ts.Node): boolean { } } -function ancestorIsExpressionStatement(origNode: ts.Node): boolean { - if (origNode.parent === undefined) { - return false; - } - return ( - origNode.parent.kind === ts.SyntaxKind.ExpressionStatement || - ancestorIsExpressionStatement(origNode.parent) - ); -} - function isStatement(node: ts.Node): node is ts.Statement { return ts.SyntaxKind.FirstStatement <= node.kind && node.kind <= ts.SyntaxKind.LastStatement; }