Merge pull request #461 from Workiva/connect-NativeJavaScriptObject-fix

CPLAT-9715 Fix accidental jsification of Map/Function Dart props when using `connect`
Workiva · Feb 18, 2020 · 7390a19 · 7390a19
2 parents 83b3d7d + 486c00d
commit 7390a19
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 2 deletions.
diff --git a/lib/src/over_react_redux/over_react_redux.dart b/lib/src/over_react_redux/over_react_redux.dart
@@ -229,10 +229,15 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend
       ),
     )(dartComponentClass);
 
-    final hocJsFactoryProxy = ReactJsComponentFactoryProxy(hoc, shouldConvertDomProps: false, alwaysReturnChildrenAsList: true);
+    /// Use a Dart proxy instead of a JS one since we're treating it like a Dart component:
+    /// props values should be passed to the underlying component (e.g., those returned by mapStateToProps)
+    /// without any conversion needed by JS Components, and props are are fed directly
+    /// into Dart code (e.g., those passed into mapStateToPropsWithOwnProps/areOwnPropsEqual)
+    /// without needing unwrapping/conversion.
+    final hocFactoryProxy = ReactDartComponentFactoryProxy2(hoc);
 
     TProps connectedFactory([Map props]) {
-      return (factory(props)..componentFactory = hocJsFactoryProxy);
+      return (factory(props)..componentFactory = hocFactoryProxy);
     }
 
     return connectedFactory;

diff --git a/test/over_react_redux/connect_test.dart b/test/over_react_redux/connect_test.dart
@@ -129,6 +129,53 @@ main() {
         expect(counterRef.current.props.currentCount, 1);
         expect(jacket.mountNode.innerHtml, contains('Count: 1'));
       });
+
+      test('without converting the props whatsoever', () {
+        // Test functions/Maps to ensure they're not allowInterop'd,
+        // test event handlers to ensure they're not oterwise converted
+        testFunction() => 'foo';
+        const testMap = {'foo': 'bar'};
+        testEventHandler(SyntheticMouseEvent e) {}
+
+        ConnectedCounter = connect<CounterState, CounterProps>(mapStateToProps: (state){
+          return (Counter()
+            ..currentCount = state.count
+            ..['functionPropSetInsideHoc'] = testFunction
+            ..['mapPropSetInsideHoc'] = testMap
+            // We need to set a real event prop key to properly test this
+            ..onMouseDown = testEventHandler
+          );
+        }, forwardRef: true)(Counter);
+
+        jacket = mount(
+          (ReduxProvider()..store = store1)(
+            (ConnectedCounter()
+              ..ref = counterRef
+              ..['functionPropSetOnHoc'] = testFunction
+              ..['mapPropSetOnHoc'] = testMap
+              // We need to set a real event prop key to properly test this
+              ..onMouseUp = testEventHandler
+            )('test'),
+          ),
+        );
+
+        final actualProps = counterRef.current.props;
+        final expectedProps = {
+          'functionPropSetInsideHoc': same(testFunction),
+          'mapPropSetInsideHoc': same(testMap),
+          'onMouseDown': same(testEventHandler),
+
+          'functionPropSetOnHoc': same(testFunction),
+          'mapPropSetOnHoc': same(testMap),
+          'onMouseUp': same(testEventHandler),
+        };
+        // Filter out unrelated props that prevent us from using the default Map matcher
+        final relevantPropKeys = {...actualProps}
+            ..removeWhere((key, value) => !expectedProps.containsKey(key));
+
+        expect(relevantPropKeys, expectedProps,
+            reason: 'functions/maps should not be jsified, and event handlers should not be converted');
+      });
     });
 
     group('mapStateToPropsWithOwnProps properly maps the state to the components props', (){