From 7c6707c03724d8af7ef8524f422378f78a876bb0 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Wed, 16 Oct 2024 11:20:46 -0700 Subject: [PATCH 01/10] Add initial test --- .../connect_required_props_test.dart | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart diff --git a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart new file mode 100644 index 00000000..dd2dcf6c --- /dev/null +++ b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart @@ -0,0 +1,97 @@ +// Copyright 2024 Workiva Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/connect_required_props.dart'; +import 'package:test/test.dart'; + +import '../../resolved_file_context.dart'; +import '../../util.dart'; +import '../../util/component_usage_migrator_test.dart'; + +void main() { + final resolvedContext = SharedAnalysisContext.overReact; + + // Warm up analysis in a setUpAll so that if getting the resolved AST times out + // (which is more common for the WSD context), it fails here instead of failing the first test. + setUpAll(resolvedContext.warmUpAnalysis); + + group('ConnectRequiredProps', () { + late SuggestorTester testSuggestor; + + setUp(() { + testSuggestor = getSuggestorTester( + ConnectRequiredProps(), + resolvedContext: resolvedContext, + ); + }); + + test( + 'adds all connect props to disable required prop validation list', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + import 'package:over_react/over_react_redux.dart'; + + // ignore: uri_has_not_been_generated + part 'main.over_react.g.dart'; + + class FooState { + /*late*/ num count; + } + + mixin FooProps on UiProps { + /*late*/ num count; + Function()/*?*/ increment; + String abc; + } + + UiFactory Foo = connect( + mapStateToProps: (state) => (Foo() + ..count = state.count + ), + mapDispatchToProps: (dispatch) => (Foo() + ..increment = (() => null) + ), + )(_\$Foo); + '''), + expectedOutput: withOverReactImport(''' + import 'package:over_react/over_react_redux.dart'; + + part 'main.over_react.g.dart'; + + class FooState { + /*late*/ num count; + } + + @Props(disableRequiredPropValidation: {'count', 'increment'}) + mixin FooProps on UiProps { + /*late*/ num count; + String/*?*/ increment; + String abc; + } + + UiFactory Foo = connect( + mapStateToProps: (state) => (Foo() + ..count = state.count + ), + mapDispatchToProps: (dispatch) => (Foo() + ..increment = (() => dispatch(IncrementAction())) + ), + )(_\$Foo); + '''), + ); + }); + }); +} From e5d9be83a8a08785cbc3672563c427901f97e0f7 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Fri, 18 Oct 2024 09:46:02 -0700 Subject: [PATCH 02/10] Add initial implementation --- ...lass_component_required_default_props.dart | 7 +- .../connect_required_props.dart | 92 +++++++++++++++++++ .../null_safety_prep/utils/props_utils.dart | 11 +++ 3 files changed, 105 insertions(+), 5 deletions(-) create mode 100644 lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart create mode 100644 lib/src/dart3_suggestors/null_safety_prep/utils/props_utils.dart diff --git a/lib/src/dart3_suggestors/null_safety_prep/class_component_required_default_props.dart b/lib/src/dart3_suggestors/null_safety_prep/class_component_required_default_props.dart index 0cf76e70..20337b91 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/class_component_required_default_props.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/class_component_required_default_props.dart @@ -15,6 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/utils/props_utils.dart'; import 'package:over_react_codemod/src/util.dart'; import 'package:over_react_codemod/src/util/component_usage.dart'; import 'package:analyzer/dart/ast/ast.dart'; @@ -99,11 +100,7 @@ class ClassComponentRequiredDefaultPropsMigrator // If this cascade is not assigning values to defaultProps, bail. if (!isDefaultProps) return; - final cascadedDefaultProps = node.cascadeSections - .whereType() - .where((assignment) => assignment.leftHandSide is PropertyAccess) - .map((assignment) => PropAssignment(assignment)) - .where((prop) => prop.node.writeElement?.displayName != null); + final cascadedDefaultProps = getCascadedProps(node); patchFieldDeclarations(getAllProps, cascadedDefaultProps, node); } diff --git a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart new file mode 100644 index 00000000..21f04e14 --- /dev/null +++ b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart @@ -0,0 +1,92 @@ +// Copyright 2024 Workiva Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import 'package:analyzer/dart/analysis/results.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/utils/props_utils.dart'; +import 'package:over_react_codemod/src/util.dart'; +import 'package:over_react_codemod/src/util/class_suggestor.dart'; + +import 'analyzer_plugin_utils.dart'; + +// todo update +/// Suggestor that replaces a `null` literal argument passed to a "DOM" callback +/// with a generated `SyntheticEvent` object of the expected type. +/// +/// Example: +/// +/// ```dart +/// final props = domProps(); +/// // Before +/// props.onClick(null); +/// // After +/// props.onClick(createSyntheticMouseEvent()); +/// ``` +class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { + late ResolvedUnitResult _result; + + @override + visitCascadeExpression(CascadeExpression node) { + super.visitCascadeExpression(node); + + // Verify the builder usage is within the `connect` method call. + final connect = node.thisOrAncestorMatching((n) => n is MethodInvocation && n.methodName.name == 'connect'); + if(connect == null) return; + + // Verify the builder usage is within one of the targeted connect args. + final connectArgs = connect.argumentList.arguments.whereType(); + final connectArg = node.thisOrAncestorMatching((n) => n is NamedExpression && connectArgs.contains(n) && connectArgNames.contains(n.name.label.name)); + if(connectArg == null) return; + + final cascadedProps = getCascadedProps(node).toList(); + + final ignoredPropsByMixin = >{}; + for (final field in cascadedProps) { + final propsElement = + node.staticType?.typeOrBound.tryCast()?.element; + if (propsElement == null) continue; + + final fieldName = field.name.name; + if (ignoredPropsByMixin[propsElement] != null) { + ignoredPropsByMixin[propsElement]!.add(fieldName); + } else { + ignoredPropsByMixin[propsElement] = [fieldName]; + } + } + + for(final propsClass in ignoredPropsByMixin.keys) { + final classNode = NodeLocator2(propsClass.nameOffset).searchWithin(_result.unit); + if(classNode != null) { + yieldPatch( + '@Props(disableRequiredPropValidation: {${ignoredPropsByMixin[propsClass]!.map((p) => '\'$p\'').join(', ')}})', + classNode.offset, classNode.offset); + } + } + } + + @override + Future generatePatches() async { + final r = await context.getResolvedUnit(); + if (r == null) { + throw Exception( + 'Could not get resolved result for "${context.relativePath}"'); + } + _result = r; + _result.unit.accept(this); + } + static const connectArgNames = ['mapStateToProps', 'mapDispatchToProps']; +} diff --git a/lib/src/dart3_suggestors/null_safety_prep/utils/props_utils.dart b/lib/src/dart3_suggestors/null_safety_prep/utils/props_utils.dart new file mode 100644 index 00000000..a902342c --- /dev/null +++ b/lib/src/dart3_suggestors/null_safety_prep/utils/props_utils.dart @@ -0,0 +1,11 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:over_react_codemod/src/util/component_usage.dart'; + +/// Returns a list of props from [cascade]. +Iterable getCascadedProps(CascadeExpression cascade) { + return cascade.cascadeSections + .whereType() + .where((assignment) => assignment.leftHandSide is PropertyAccess) + .map((assignment) => PropAssignment(assignment)) + .where((prop) => prop.node.writeElement?.displayName != null); +} From 2ee50987f9c8e3a256c0cc5c918b8bd9da8e91e1 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Fri, 18 Oct 2024 15:22:19 -0700 Subject: [PATCH 03/10] Add fix for existing annotations --- .../connect_required_props.dart | 97 ++++++-- .../connect_required_props_test.dart | 229 +++++++++++++++--- 2 files changed, 275 insertions(+), 51 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart index 21f04e14..cb7befae 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart @@ -12,11 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -import 'package:analyzer/dart/analysis/results.dart'; import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/type.dart'; +import 'package:collection/collection.dart'; import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/utils/props_utils.dart'; import 'package:over_react_codemod/src/util.dart'; import 'package:over_react_codemod/src/util/class_suggestor.dart'; @@ -37,56 +37,105 @@ import 'analyzer_plugin_utils.dart'; /// props.onClick(createSyntheticMouseEvent()); /// ``` class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { - late ResolvedUnitResult _result; + /// Running list of props that should be ignored per mixin that will all be added + /// at the end in [generatePatches]. + final _ignoredPropsByMixin = >{}; @override visitCascadeExpression(CascadeExpression node) { super.visitCascadeExpression(node); // Verify the builder usage is within the `connect` method call. - final connect = node.thisOrAncestorMatching((n) => n is MethodInvocation && n.methodName.name == 'connect'); - if(connect == null) return; + final connect = node.thisOrAncestorMatching( + (n) => n is MethodInvocation && n.methodName.name == 'connect'); + if (connect == null) return; // Verify the builder usage is within one of the targeted connect args. - final connectArgs = connect.argumentList.arguments.whereType(); - final connectArg = node.thisOrAncestorMatching((n) => n is NamedExpression && connectArgs.contains(n) && connectArgNames.contains(n.name.label.name)); - if(connectArg == null) return; + final connectArgs = + connect.argumentList.arguments.whereType(); + final connectArg = node.thisOrAncestorMatching((n) => + n is NamedExpression && + connectArgs.contains(n) && + connectArgNames.contains(n.name.label.name)); + if (connectArg == null) return; final cascadedProps = getCascadedProps(node).toList(); - final ignoredPropsByMixin = >{}; for (final field in cascadedProps) { final propsElement = node.staticType?.typeOrBound.tryCast()?.element; if (propsElement == null) continue; final fieldName = field.name.name; - if (ignoredPropsByMixin[propsElement] != null) { - ignoredPropsByMixin[propsElement]!.add(fieldName); + if (_ignoredPropsByMixin[propsElement] != null) { + _ignoredPropsByMixin[propsElement]!.add(fieldName); } else { - ignoredPropsByMixin[propsElement] = [fieldName]; - } - } - - for(final propsClass in ignoredPropsByMixin.keys) { - final classNode = NodeLocator2(propsClass.nameOffset).searchWithin(_result.unit); - if(classNode != null) { - yieldPatch( - '@Props(disableRequiredPropValidation: {${ignoredPropsByMixin[propsClass]!.map((p) => '\'$p\'').join(', ')}})', - classNode.offset, classNode.offset); + _ignoredPropsByMixin[propsElement] = [fieldName]; } } } @override Future generatePatches() async { - final r = await context.getResolvedUnit(); - if (r == null) { + _ignoredPropsByMixin.clear(); + final result = await context.getResolvedUnit(); + if (result == null) { throw Exception( 'Could not get resolved result for "${context.relativePath}"'); } - _result = r; - _result.unit.accept(this); + result.unit.accept(this); + + // Add the patches at the end so that all the props to be ignored can be collected + // from the different args in `connect` before adding patches to avoid duplicate patches. + for (final propsClass in _ignoredPropsByMixin.keys) { + final propsToIgnore = _ignoredPropsByMixin[propsClass]!; + final classNode = + NodeLocator2(propsClass.nameOffset).searchWithin(result.unit); + if (classNode != null && classNode is NamedCompilationUnitMember) { + final existingAnnotation = + classNode.metadata.where((c) => c.name.name == 'Props').firstOrNull; + if (existingAnnotation == null) { + yieldPatch( + '@Props(disableRequiredPropValidation: {${propsToIgnore.map((p) => '\'$p\'').join(', ')}})', + classNode.offset, + classNode.offset); + } else { + final ignoreArg = existingAnnotation.arguments?.arguments + .whereType() + .where( + (e) => e.name.label.name == 'disableRequiredPropValidation') + .firstOrNull; + if (ignoreArg == null) { + final offset = existingAnnotation.arguments?.leftParenthesis.end; + if (offset != null) { + yieldPatch( + 'disableRequiredPropValidation: {${propsToIgnore.map((p) => '\'$p\'').join(', ')}}${existingAnnotation.arguments?.arguments.isNotEmpty ?? false ? ', ' : ''}', + offset, + offset); + } + } else { + final existingList = + ignoreArg.expression.tryCast(); + if (existingList != null) { + final alreadyIgnored = existingList.elements + .whereType() + .map((e) => e.stringValue) + .toList(); + final newPropsToIgnore = + propsToIgnore.where((p) => !alreadyIgnored.contains(p)); + if (newPropsToIgnore.isNotEmpty) { + final offset = existingList.leftBracket.end; + yieldPatch( + '${newPropsToIgnore.map((p) => '\'$p\'').join(', ')}, ', + offset, + offset); + } + } + } + } + } + } } + static const connectArgNames = ['mapStateToProps', 'mapDispatchToProps']; } diff --git a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart index dd2dcf6c..f76443dd 100644 --- a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart +++ b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart @@ -26,9 +26,25 @@ void main() { // (which is more common for the WSD context), it fails here instead of failing the first test. setUpAll(resolvedContext.warmUpAnalysis); - group('ConnectRequiredProps', () { + group( + 'ConnectRequiredProps - adds all connect props to disable required prop validation list', + () { late SuggestorTester testSuggestor; + String commonConnectFile(String source) { + return ''' + $overReactImport + import 'package:over_react/over_react_redux.dart'; + + // ignore: uri_has_not_been_generated + part 'main.over_react.g.dart'; + + class FooState { + num count; + } + $source'''; + } + setUp(() { testSuggestor = getSuggestorTester( ConnectRequiredProps(), @@ -36,45 +52,170 @@ void main() { ); }); - test( - 'adds all connect props to disable required prop validation list', - () async { + test('', () async { await testSuggestor( - expectedPatchCount: 0, - input: withOverReactImport(''' - import 'package:over_react/over_react_redux.dart'; - - // ignore: uri_has_not_been_generated - part 'main.over_react.g.dart'; - - class FooState { - /*late*/ num count; + input: commonConnectFile(''' + mixin FooProps on UiProps { + num count; + Function() increment; + String abc; } + UiFactory Foo = connect( + mapStateToProps: (state) => (Foo() + ..addTestId('abc') + ..count = state.count + ), + mapDispatchToProps: (dispatch) => Foo()..increment = (() => null), + )(uiFunction((props) => (Foo()..abc = '1')(), _\$Foo)); + '''), + expectedOutput: commonConnectFile(''' + @Props(disableRequiredPropValidation: {'count', 'increment'}) mixin FooProps on UiProps { + num count; + Function() increment; + String abc; + } + + UiFactory Foo = connect( + mapStateToProps: (state) => (Foo() + ..addTestId('abc') + ..count = state.count + ), + mapDispatchToProps: (dispatch) => Foo()..increment = (() => null), + )(uiFunction((props) => (Foo()..abc = '1')(), _\$Foo)); + '''), + ); + }); + + test('for multiple mixins', () async { + await testSuggestor( + input: commonConnectFile(''' + mixin FooPropsMixin on UiProps { /*late*/ num count; Function()/*?*/ increment; String abc; } + mixin OtherPropsMixin on UiProps { + String a; + String b; + } + + class FooProps = UiProps with FooPropsMixin, OtherPropsMixin; + UiFactory Foo = connect( mapStateToProps: (state) => (Foo() + ..addTestId('abc') ..count = state.count + ..a = '1' ), - mapDispatchToProps: (dispatch) => (Foo() - ..increment = (() => null) + mapDispatchToProps: (dispatch) => Foo()..increment = (() => null), + )(uiFunction((props) => (Foo()..abc = '1'..b = '2')(), _\$Foo)); + '''), + expectedOutput: commonConnectFile(''' + mixin FooPropsMixin on UiProps { + /*late*/ num count; + Function()/*?*/ increment; + String abc; + } + + mixin OtherPropsMixin on UiProps { + String a; + String b; + } + + @Props(disableRequiredPropValidation: {'count', 'increment'}) + class FooProps = UiProps with FooPropsMixin, OtherPropsMixin; + + UiFactory Foo = connect( + mapStateToProps: (state) => (Foo() + ..addTestId('abc') + ..count = state.count + ..a = '1' ), - )(_\$Foo); + mapDispatchToProps: (dispatch) => Foo()..increment = (() => null), + )(uiFunction((props) => (Foo()..abc = '1'..b = '2')(), _\$Foo)); '''), - expectedOutput: withOverReactImport(''' - import 'package:over_react/over_react_redux.dart'; - - part 'main.over_react.g.dart'; - - class FooState { + ); + }); + + group('adds to existing annotations', () { + Future testAnnotations( + {required String input, required String expectedOutput}) async { + final connectBoilerplate = ''' + mixin FooProps on UiProps { + num connectProp1; + Function() connectProp2; + String nonConnectProp; + } + + UiFactory Foo = connect( + mapStateToProps: (state) => (Foo()..connectProp1 = 1), + mapDispatchToProps: (dispatch) => Foo()..connectProp2 = (() => null), + )(uiFunction((props) => (Foo()..nonConnectProp = '1')(), _\$Foo)); + '''; + await testSuggestor( + input: commonConnectFile(''' + $input + $connectBoilerplate + '''), + expectedOutput: commonConnectFile(''' + $expectedOutput + $connectBoilerplate + '''), + ); + } + + test('', () async { + await testAnnotations( + input: '@Props()', + expectedOutput: + '@Props(disableRequiredPropValidation: {\'connectProp1\', \'connectProp2\'})', + ); + }); + + test('with other args', () async { + await testAnnotations( + input: '@Props(keyNamespace: \'\')', + expectedOutput: + '@Props(disableRequiredPropValidation: {\'connectProp1\', \'connectProp2\'}, keyNamespace: \'\')', + ); + }); + + test('with disableRequiredPropValidation', () async { + await testAnnotations( + input: '@Props(disableRequiredPropValidation: {\'connectProp1\'})', + expectedOutput: + '@Props(disableRequiredPropValidation: {\'connectProp2\', \'connectProp1\'})', + ); + }); + }); + + test('recognizes different arg formats', () async { + await testSuggestor( + input: commonConnectFile(''' + mixin FooProps on UiProps { /*late*/ num count; + Function()/*?*/ increment; + String abc; } + UiFactory Foo = connect( + mapStateToProps: (state) { + return (Foo() + ..count = state.count + ); + }, + mapDispatchToProps: (dispatch) { + final foo = (Foo() + ..increment = (() => null) + ); + return foo; + }, + )(_\$Foo); + '''), + expectedOutput: commonConnectFile(''' @Props(disableRequiredPropValidation: {'count', 'increment'}) mixin FooProps on UiProps { /*late*/ num count; @@ -83,12 +224,46 @@ void main() { } UiFactory Foo = connect( - mapStateToProps: (state) => (Foo() + mapStateToProps: (state) { + return (Foo() + ..count = state.count + ); + }, + mapDispatchToProps: (dispatch) { + final foo = (Foo() + ..increment = (() => dispatch(IncrementAction())) + ); + return foo; + }, + )(_\$Foo); + '''), + ); + }); + + test('does not cover certain unlikely edge cases', () async { + await testSuggestor( + expectedPatchCount: 0, + input: commonConnectFile(''' + mixin FooProps on UiProps { + /*late*/ num count; + Function()/*?*/ increment; + String abc; + } + + final _mapStateToProps = (state) { + return (Foo() ..count = state.count - ), - mapDispatchToProps: (dispatch) => (Foo() - ..increment = (() => dispatch(IncrementAction())) - ), + ); + }; + + UiFactory Foo = connect( + mapStateToProps: _mapStateToProps, + mapDispatchToProps: (dispatch) { + final foo = (Foo() + ..increment = (() => null) + ); + return foo; + }, )(_\$Foo); '''), ); From 95d4ce2ef5513504f77ddda78c54793a1f527d76 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Mon, 21 Oct 2024 11:59:15 -0700 Subject: [PATCH 04/10] Clean up tests --- .../connect_required_props_test.dart | 138 +++++++----------- 1 file changed, 49 insertions(+), 89 deletions(-) diff --git a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart index f76443dd..6218da38 100644 --- a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart +++ b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart @@ -53,89 +53,61 @@ void main() { }); test('', () async { - await testSuggestor( - input: commonConnectFile(''' + final input = ''' mixin FooProps on UiProps { - num count; - Function() increment; - String abc; + num setInMapStateToProps; + Function() SetInMapDispatchToProps; + String notSetInConnect; } UiFactory Foo = connect( mapStateToProps: (state) => (Foo() ..addTestId('abc') - ..count = state.count + ..setInMapStateToProps = state.count ), - mapDispatchToProps: (dispatch) => Foo()..increment = (() => null), - )(uiFunction((props) => (Foo()..abc = '1')(), _\$Foo)); - '''), + mapDispatchToProps: (dispatch) => Foo()..SetInMapDispatchToProps = (() => null), + )(uiFunction((props) => (Foo()..notSetInConnect = '1')(), _\$Foo)); + '''; + + await testSuggestor( + input: commonConnectFile(input), expectedOutput: commonConnectFile(''' - @Props(disableRequiredPropValidation: {'count', 'increment'}) - mixin FooProps on UiProps { - num count; - Function() increment; - String abc; - } - - UiFactory Foo = connect( - mapStateToProps: (state) => (Foo() - ..addTestId('abc') - ..count = state.count - ), - mapDispatchToProps: (dispatch) => Foo()..increment = (() => null), - )(uiFunction((props) => (Foo()..abc = '1')(), _\$Foo)); + @Props(disableRequiredPropValidation: {'setInMapStateToProps', 'SetInMapDispatchToProps'}) + $input '''), ); }); test('for multiple mixins', () async { - await testSuggestor( - input: commonConnectFile(''' - mixin FooPropsMixin on UiProps { - /*late*/ num count; - Function()/*?*/ increment; - String abc; - } - - mixin OtherPropsMixin on UiProps { - String a; - String b; - } - + final input = ''' class FooProps = UiProps with FooPropsMixin, OtherPropsMixin; - UiFactory Foo = connect( - mapStateToProps: (state) => (Foo() - ..addTestId('abc') - ..count = state.count - ..a = '1' - ), - mapDispatchToProps: (dispatch) => Foo()..increment = (() => null), - )(uiFunction((props) => (Foo()..abc = '1'..b = '2')(), _\$Foo)); - '''), - expectedOutput: commonConnectFile(''' mixin FooPropsMixin on UiProps { - /*late*/ num count; - Function()/*?*/ increment; - String abc; + /*late*/ num prop1; + Function()/*?*/ prop2; + String notSetInConnect; } mixin OtherPropsMixin on UiProps { - String a; - String b; + String otherProp; + String notSetInConnect2; } - @Props(disableRequiredPropValidation: {'count', 'increment'}) - class FooProps = UiProps with FooPropsMixin, OtherPropsMixin; - UiFactory Foo = connect( mapStateToProps: (state) => (Foo() ..addTestId('abc') - ..count = state.count - ..a = '1' + ..prop1 = state.count + ..otherProp = '1' ), - mapDispatchToProps: (dispatch) => Foo()..increment = (() => null), - )(uiFunction((props) => (Foo()..abc = '1'..b = '2')(), _\$Foo)); + mapDispatchToProps: (dispatch) => Foo()..prop2 = (() => null), + )(uiFunction((props) => (Foo()..notSetInConnect = '1'..notSetInConnect2 = '2')(), _\$Foo)); + '''; + + await testSuggestor( + input: commonConnectFile(input), + expectedOutput: commonConnectFile(''' + @Props(disableRequiredPropValidation: {'prop1', 'otherProp', 'prop2'}) + $input '''), ); }); @@ -193,8 +165,7 @@ void main() { }); test('recognizes different arg formats', () async { - await testSuggestor( - input: commonConnectFile(''' + final input = ''' mixin FooProps on UiProps { /*late*/ num count; Function()/*?*/ increment; @@ -214,45 +185,27 @@ void main() { return foo; }, )(_\$Foo); - '''), + '''; + await testSuggestor( + input: commonConnectFile(input), expectedOutput: commonConnectFile(''' @Props(disableRequiredPropValidation: {'count', 'increment'}) - mixin FooProps on UiProps { - /*late*/ num count; - String/*?*/ increment; - String abc; - } - - UiFactory Foo = connect( - mapStateToProps: (state) { - return (Foo() - ..count = state.count - ); - }, - mapDispatchToProps: (dispatch) { - final foo = (Foo() - ..increment = (() => dispatch(IncrementAction())) - ); - return foo; - }, - )(_\$Foo); + $input '''), ); }); test('does not cover certain unlikely edge cases', () async { - await testSuggestor( - expectedPatchCount: 0, - input: commonConnectFile(''' + final input = ''' mixin FooProps on UiProps { - /*late*/ num count; - Function()/*?*/ increment; - String abc; + num inTearOff; + Function() notReturned; + String notUsed; } final _mapStateToProps = (state) { return (Foo() - ..count = state.count + ..inTearOff = state.count ); }; @@ -260,11 +213,18 @@ void main() { mapStateToProps: _mapStateToProps, mapDispatchToProps: (dispatch) { final foo = (Foo() - ..increment = (() => null) + ..notReturned = (() => null) ); - return foo; + foo; + return Foo(); }, )(_\$Foo); + '''; + await testSuggestor( + input: commonConnectFile(input), + expectedOutput: commonConnectFile(''' + @Props(disableRequiredPropValidation: {'notReturned'}) + $input '''), ); }); From ce4334fc7122fda2d8c3d878daa548026fc7b606 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Mon, 21 Oct 2024 12:23:41 -0700 Subject: [PATCH 05/10] Update comments --- .../connect_required_props.dart | 36 +++++++++---------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart index cb7befae..f200c374 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart @@ -23,19 +23,8 @@ import 'package:over_react_codemod/src/util/class_suggestor.dart'; import 'analyzer_plugin_utils.dart'; -// todo update -/// Suggestor that replaces a `null` literal argument passed to a "DOM" callback -/// with a generated `SyntheticEvent` object of the expected type. -/// -/// Example: -/// -/// ```dart -/// final props = domProps(); -/// // Before -/// props.onClick(null); -/// // After -/// props.onClick(createSyntheticMouseEvent()); -/// ``` +/// Suggestor that adds `@Props(disableRequiredPropValidation: {...})` annotations +/// for props that are set in `connect` components. class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { /// Running list of props that should be ignored per mixin that will all be added /// at the end in [generatePatches]. @@ -66,6 +55,7 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { node.staticType?.typeOrBound.tryCast()?.element; if (propsElement == null) continue; + // Keep a running list of props to ignore per props mixin. final fieldName = field.name.name; if (_ignoredPropsByMixin[propsElement] != null) { _ignoredPropsByMixin[propsElement]!.add(fieldName); @@ -94,28 +84,33 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { if (classNode != null && classNode is NamedCompilationUnitMember) { final existingAnnotation = classNode.metadata.where((c) => c.name.name == 'Props').firstOrNull; + if (existingAnnotation == null) { + // Add full @Props annotation if it doesn't exist. yieldPatch( - '@Props(disableRequiredPropValidation: {${propsToIgnore.map((p) => '\'$p\'').join(', ')}})', + '@Props($annotationArg: {${propsToIgnore.map((p) => '\'$p\'').join(', ')}})', classNode.offset, classNode.offset); } else { - final ignoreArg = existingAnnotation.arguments?.arguments + final existingAnnotationArg = existingAnnotation.arguments?.arguments .whereType() - .where( - (e) => e.name.label.name == 'disableRequiredPropValidation') + .where((e) => e.name.label.name == annotationArg) .firstOrNull; - if (ignoreArg == null) { + + if (existingAnnotationArg == null) { + // Add disable validation arg to existing @Props annotation. final offset = existingAnnotation.arguments?.leftParenthesis.end; if (offset != null) { yieldPatch( - 'disableRequiredPropValidation: {${propsToIgnore.map((p) => '\'$p\'').join(', ')}}${existingAnnotation.arguments?.arguments.isNotEmpty ?? false ? ', ' : ''}', + '$annotationArg: {${propsToIgnore.map((p) => '\'$p\'').join(', ')}}${existingAnnotation.arguments?.arguments.isNotEmpty ?? false ? ', ' : ''}', offset, offset); } } else { + // Add props to disable validation for to the existing list of disabled + // props in the @Props annotation if they aren't already listed. final existingList = - ignoreArg.expression.tryCast(); + existingAnnotationArg.expression.tryCast(); if (existingList != null) { final alreadyIgnored = existingList.elements .whereType() @@ -138,4 +133,5 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { } static const connectArgNames = ['mapStateToProps', 'mapDispatchToProps']; + static const annotationArg = 'disableRequiredPropValidation'; } From 65eae330fc9e9f553e048c7416d460b56ea9ac07 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Tue, 22 Oct 2024 16:18:35 -0700 Subject: [PATCH 06/10] Add to list of connect args supported --- .../null_safety_prep/connect_required_props.dart | 7 ++++++- lib/src/executables/null_safety_migrator_companion.dart | 5 ++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart index f200c374..b9d6ebd8 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart @@ -132,6 +132,11 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { } } - static const connectArgNames = ['mapStateToProps', 'mapDispatchToProps']; + static const connectArgNames = [ + 'mapStateToProps', + 'mapStateToPropsWithOwnProps', + 'mapDispatchToProps', + 'mapDispatchToPropsWithOwnProps', + ]; static const annotationArg = 'disableRequiredPropValidation'; } diff --git a/lib/src/executables/null_safety_migrator_companion.dart b/lib/src/executables/null_safety_migrator_companion.dart index 87567ab0..a00d21a9 100644 --- a/lib/src/executables/null_safety_migrator_companion.dart +++ b/lib/src/executables/null_safety_migrator_companion.dart @@ -18,6 +18,7 @@ import 'package:args/args.dart'; import 'package:codemod/codemod.dart'; import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/class_component_required_default_props.dart'; import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/class_component_required_initial_state.dart'; +import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/connect_required_props.dart'; import 'package:over_react_codemod/src/util.dart'; import '../dart3_suggestors/null_safety_prep/callback_ref_hint_suggestor.dart'; @@ -42,9 +43,7 @@ void main(List args) async { exitCode = await runInteractiveCodemod( dartPaths, aggregate([ - CallbackRefHintSuggestor(), - ClassComponentRequiredDefaultPropsMigrator(), - ClassComponentRequiredInitialStateMigrator(), + ConnectRequiredProps(), ]), defaultYes: true, args: parsedArgs.rest, From 476d255b8de32065b06cb7fc4337aa6e2327c237 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Tue, 22 Oct 2024 16:19:17 -0700 Subject: [PATCH 07/10] Add to list of connect args supported --- .../connect_required_props_test.dart | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart index 6218da38..58fbdc32 100644 --- a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart +++ b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart @@ -195,6 +195,34 @@ void main() { ); }); + test('only adds props used in specific connect args', () async { + final input = ''' + mixin FooProps on UiProps { + num propInMapStateToProps; + num propInMapStateToPropsWithOwnProps; + num propInMapDispatchToProps; + num propInMapDispatchToPropsWithOwnProps; + num propInMergeProps; + String notUsed; + } + + UiFactory Foo = connect( + mapStateToProps: (_) => (Foo()..propInMapStateToProps = 1), + mapStateToPropsWithOwnProps: (_, __) => (Foo()..propInMapStateToPropsWithOwnProps = 1), + mapDispatchToProps: (_) => (Foo()..propInMapDispatchToProps = 1), + mapDispatchToPropsWithOwnProps: (_, __) => (Foo()..propInMapDispatchToPropsWithOwnProps = 1), + mergeProps: (_, __, ___) => (Foo()..propInMergeProps = 1), + )(_\$Foo); + '''; + await testSuggestor( + input: commonConnectFile(input), + expectedOutput: commonConnectFile(''' + @Props(disableRequiredPropValidation: {'propInMapStateToProps', 'propInMapStateToPropsWithOwnProps', 'propInMapDispatchToProps', 'propInMapDispatchToPropsWithOwnProps'}) + $input + '''), + ); + }); + test('does not cover certain unlikely edge cases', () async { final input = ''' mixin FooProps on UiProps { From d5b3150ce54b4a3211da4c2ac11a9b356dc6ef6e Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Tue, 22 Oct 2024 16:38:06 -0700 Subject: [PATCH 08/10] Add suggestor to migrator companion script --- .../null_safety_prep/connect_required_props.dart | 2 +- .../null_safety_migrator_companion.dart | 14 ++++++++++++++ .../null_safety_migrator_companion_test.dart | 2 ++ .../test_package/lib/src/test_state.dart | 7 +++++-- 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart index b9d6ebd8..1fdf9743 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart @@ -88,7 +88,7 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { if (existingAnnotation == null) { // Add full @Props annotation if it doesn't exist. yieldPatch( - '@Props($annotationArg: {${propsToIgnore.map((p) => '\'$p\'').join(', ')}})', + '@Props($annotationArg: {${propsToIgnore.map((p) => '\'$p\'').join(', ')}})\n', classNode.offset, classNode.offset); } else { diff --git a/lib/src/executables/null_safety_migrator_companion.dart b/lib/src/executables/null_safety_migrator_companion.dart index 9ea91769..cf7335f6 100644 --- a/lib/src/executables/null_safety_migrator_companion.dart +++ b/lib/src/executables/null_safety_migrator_companion.dart @@ -17,6 +17,7 @@ import 'dart:io'; import 'package:args/args.dart'; import 'package:codemod/codemod.dart'; import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/class_component_required_initial_state.dart'; +import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/connect_required_props.dart'; import 'package:over_react_codemod/src/util.dart'; import '../dart3_suggestors/null_safety_prep/callback_ref_hint_suggestor.dart'; @@ -78,4 +79,17 @@ void main(List args) async { additionalHelpOutput: parser.usage, changesRequiredOutput: _changesRequiredOutput, ); + + if (exitCode != 0) return; + + exitCode = await runInteractiveCodemodSequence( + dartPaths, + [ + ConnectRequiredProps(), + ], + defaultYes: true, + args: parsedArgs.rest, + additionalHelpOutput: parser.usage, + changesRequiredOutput: _changesRequiredOutput, + ); } diff --git a/test/executables/null_safety_migrator_companion_test.dart b/test/executables/null_safety_migrator_companion_test.dart index d7cdb3cf..55f60fe0 100644 --- a/test/executables/null_safety_migrator_companion_test.dart +++ b/test/executables/null_safety_migrator_companion_test.dart @@ -46,8 +46,10 @@ void main() { d.dir('lib', [ d.dir('src', [ d.file('test_state.dart', contains(''' +@Props(disableRequiredPropValidation: {\'prop1\'}) mixin FooProps on UiProps { int prop1; + int prop2; } mixin FooState on UiState { diff --git a/test/test_fixtures/required_props/test_package/lib/src/test_state.dart b/test/test_fixtures/required_props/test_package/lib/src/test_state.dart index 73a547ad..8d33b829 100644 --- a/test/test_fixtures/required_props/test_package/lib/src/test_state.dart +++ b/test/test_fixtures/required_props/test_package/lib/src/test_state.dart @@ -1,15 +1,18 @@ import 'dart:html'; import 'package:over_react/over_react.dart'; +import 'package:over_react/over_react_redux.dart'; // ignore: uri_has_not_been_generated part 'test_state.over_react.g.dart'; -UiFactory Foo = - castUiFactory(_$Foo); // ignore: undefined_identifier +UiFactory Foo = connect( + mapStateToPropsWithOwnProps: (state, props) => Foo()..prop1 = 1, +)(castUiFactory(_$Hoc)); // ignore: undefined_identifier mixin FooProps on UiProps { int prop1; + int prop2; } mixin FooState on UiState { From 695352388d1d37d613bcc7d91f4e4db613776117 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Tue, 22 Oct 2024 17:00:25 -0700 Subject: [PATCH 09/10] Add one more test case --- .../null_safety_prep/connect_required_props.dart | 4 +++- .../null_safety_prep/connect_required_props_test.dart | 8 +++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart index 1fdf9743..1b99277d 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart @@ -58,7 +58,9 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { // Keep a running list of props to ignore per props mixin. final fieldName = field.name.name; if (_ignoredPropsByMixin[propsElement] != null) { - _ignoredPropsByMixin[propsElement]!.add(fieldName); + if (!_ignoredPropsByMixin[propsElement]!.contains(fieldName)) { + _ignoredPropsByMixin[propsElement]!.add(fieldName); + } } else { _ignoredPropsByMixin[propsElement] = [fieldName]; } diff --git a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart index 58fbdc32..6a3894f6 100644 --- a/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart +++ b/test/dart3_suggestors/null_safety_prep/connect_required_props_test.dart @@ -56,7 +56,8 @@ void main() { final input = ''' mixin FooProps on UiProps { num setInMapStateToProps; - Function() SetInMapDispatchToProps; + Function() setInMapDispatchToProps; + num setInBoth; String notSetInConnect; } @@ -64,15 +65,16 @@ void main() { mapStateToProps: (state) => (Foo() ..addTestId('abc') ..setInMapStateToProps = state.count + ..setInBoth = 1 ), - mapDispatchToProps: (dispatch) => Foo()..SetInMapDispatchToProps = (() => null), + mapDispatchToProps: (dispatch) => Foo()..setInMapDispatchToProps = (() => null)..setInBoth = 1, )(uiFunction((props) => (Foo()..notSetInConnect = '1')(), _\$Foo)); '''; await testSuggestor( input: commonConnectFile(input), expectedOutput: commonConnectFile(''' - @Props(disableRequiredPropValidation: {'setInMapStateToProps', 'SetInMapDispatchToProps'}) + @Props(disableRequiredPropValidation: {'setInMapStateToProps', 'setInBoth', 'setInMapDispatchToProps'}) $input '''), ); From 2e2e6848af849430f9d06ba7fc01c7a00bd2ecd0 Mon Sep 17 00:00:00 2001 From: Sydney Jodon Date: Wed, 23 Oct 2024 14:19:11 -0700 Subject: [PATCH 10/10] Address feedback --- .../null_safety_prep/connect_required_props.dart | 15 ++++----------- .../test_package/lib/src/test_state.dart | 2 +- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart index 1b99277d..11d7f458 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/connect_required_props.dart @@ -28,7 +28,7 @@ import 'analyzer_plugin_utils.dart'; class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { /// Running list of props that should be ignored per mixin that will all be added /// at the end in [generatePatches]. - final _ignoredPropsByMixin = >{}; + final _ignoredPropsByMixin = >{}; @override visitCascadeExpression(CascadeExpression node) { @@ -57,13 +57,7 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { // Keep a running list of props to ignore per props mixin. final fieldName = field.name.name; - if (_ignoredPropsByMixin[propsElement] != null) { - if (!_ignoredPropsByMixin[propsElement]!.contains(fieldName)) { - _ignoredPropsByMixin[propsElement]!.add(fieldName); - } - } else { - _ignoredPropsByMixin[propsElement] = [fieldName]; - } + _ignoredPropsByMixin.putIfAbsent(propsElement, () => {}).add(fieldName); } } @@ -79,8 +73,7 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { // Add the patches at the end so that all the props to be ignored can be collected // from the different args in `connect` before adding patches to avoid duplicate patches. - for (final propsClass in _ignoredPropsByMixin.keys) { - final propsToIgnore = _ignoredPropsByMixin[propsClass]!; + _ignoredPropsByMixin.forEach((propsClass, propsToIgnore) { final classNode = NodeLocator2(propsClass.nameOffset).searchWithin(result.unit); if (classNode != null && classNode is NamedCompilationUnitMember) { @@ -131,7 +124,7 @@ class ConnectRequiredProps extends RecursiveAstVisitor with ClassSuggestor { } } } - } + }); } static const connectArgNames = [ diff --git a/test/test_fixtures/required_props/test_package/lib/src/test_state.dart b/test/test_fixtures/required_props/test_package/lib/src/test_state.dart index 8d33b829..e2262318 100644 --- a/test/test_fixtures/required_props/test_package/lib/src/test_state.dart +++ b/test/test_fixtures/required_props/test_package/lib/src/test_state.dart @@ -8,7 +8,7 @@ part 'test_state.over_react.g.dart'; UiFactory Foo = connect( mapStateToPropsWithOwnProps: (state, props) => Foo()..prop1 = 1, -)(castUiFactory(_$Hoc)); // ignore: undefined_identifier +)(castUiFactory(_$Foo)); // ignore: undefined_identifier mixin FooProps on UiProps { int prop1;