CPLAT-9720 Connect perf optimizations

lib/src/component_declaration/component_base.dart

@@ -541,7 +541,10 @@ abstract class UiProps extends MapBase
     // Use `identical` since it compiles down to `===` in dart2js instead of calling equality helper functions,
     // and we don't want to allow any object overriding `operator==` to claim it's equal to `_notSpecified`.
     if (identical(c1, notSpecified)) {
-      childArguments = [];
+      // Use a const list so that empty children prop values are always identical
+      // in the JS props, resulting in JS libraries (e.g., react-redux) and Dart code alike
+      // not marking props as having changed as a result of rerendering the ReactElement with a new list.
+      childArguments = const [];
     } else if (identical(c2, notSpecified)) {
       childArguments = [c1];
     } else if (identical(c3, notSpecified)) {

lib/src/over_react_redux/over_react_flux.dart

@@ -15,10 +15,10 @@
 import 'dart:async';
 import 'dart:html';
 
-import 'package:collection/collection.dart';
 import 'package:meta/meta.dart';
 import 'package:over_react/over_react.dart';
 import 'package:over_react/over_react_redux.dart';
+import 'package:over_react/src/util/equality.dart';
 import 'package:redux/redux.dart' as redux;
 import 'package:w_flux/w_flux.dart' as flux;
 
@@ -309,8 +309,6 @@ mixin InfluxStoreMixin<S> on flux.Store {
   }
 }
 
-bool _shallowMapEquality(Map a, Map b) => const MapEquality().equals(a, b);
-
 /// A wrapper around the `connect` function that provides a similar API into a Flux store.
 ///
 /// This is primarily for use while transitioning _to_ `connect` and OverReact Redux.
@@ -357,9 +355,9 @@ bool _shallowMapEquality(Map a, Map b) => const MapEquality().equals(a, b);
 /// If you do not provide [mergeProps], the wrapped component receives {...ownProps, ...stateProps, ...dispatchProps}
 /// by default.
 ///
-/// - [areOwnPropsEqual] does a shallow Map equality check by default.
-/// - [areStatePropsEqual] does a shallow Map equality check by default.
-/// - [areMergedPropsEqual] does a shallow Map equality check by default.
+/// - [areOwnPropsEqual] does an equality check using JS `===` (equivalent to [identical]) by default.
+/// - [areStatePropsEqual] does a shallow Map equality check using JS `===` (equivalent to [identical]) by default.
+/// - [areMergedPropsEqual] does a shallow Map equality check using JS `===` (equivalent to [identical]) by default.
 ///
 /// - [context] can be utilized to provide a custom context object created with `createContext`.
 /// [context] is how you can utilize multiple stores. While supported, this is not recommended.
@@ -569,13 +567,13 @@ UiFactory<TProps> Function(UiFactory<TProps>)
   }
   /*--end usage of cases--*/
 
-  if (areStatePropsEqual == null) {
-    const defaultAreStatePropsEqual = _shallowMapEquality;
-    const propHasher = CollectionLengthHasher();
-    bool areStatePropsEqualWrapper(TProps nextProps, TProps prevProps) {
-      final result = defaultAreStatePropsEqual(nextProps, prevProps);
+  // In dev mode, if areStatePropsEqual is not specified, pass in a version
+  // that warns for common pitfall cases.
+  assert(() {
+    if (areStatePropsEqual == null) {
+      bool areStatePropsEqualWrapper(TProps nextProps, TProps prevProps) {
+        const propHasher = CollectionLengthHasher();
 
-      assert(() {
         prevProps.forEach((key, value) {
           // If the value is the same instance, check if the instance has been mutated,
           // causing its hash to be updated
@@ -592,14 +590,13 @@ UiFactory<TProps> Function(UiFactory<TProps>)
           }
         });
 
-        return true;
-      }());
-
-      return result;
+        return areMapsShallowIdentical(nextProps, prevProps);
+      }
+      areStatePropsEqual = areStatePropsEqualWrapper;
     }
 
-    areStatePropsEqual = areStatePropsEqualWrapper;
-  }
+    return true;
+  }());
 
   return connect(
     mapStateToProps: mapStateToProps,

lib/src/over_react_redux/over_react_redux.dart

@@ -18,16 +18,16 @@ library over_react_redux;
 
 import 'dart:html';
 import 'dart:js_util' as js_util;
-import 'package:meta/meta.dart';
-import 'package:over_react/src/component_declaration/component_base.dart' as component_base;
-import 'package:over_react/src/component_declaration/builder_helpers.dart' as builder_helpers;
-import 'package:collection/collection.dart';
+
 import 'package:js/js.dart';
+import 'package:memoize/memoize.dart';
+import 'package:meta/meta.dart';
 import 'package:over_react/over_react.dart';
+import 'package:over_react/src/component_declaration/builder_helpers.dart' as builder_helpers;
 import 'package:over_react/src/component_declaration/component_type_checking.dart';
 import 'package:react/react_client.dart';
-import 'package:react/react_client/react_interop.dart';
 import 'package:react/react_client/js_backed_map.dart';
+import 'package:react/react_client/react_interop.dart';
 import 'package:redux/redux.dart';
 
 part 'over_react_redux.over_react.g.dart';
@@ -89,10 +89,10 @@ typedef dynamic Dispatcher(dynamic action);
 /// If you do not provide [mergeProps], the wrapped component receives {...ownProps, ...stateProps, ...dispatchProps}
 /// by default.
 ///
-/// - [areStatesEqual] does a simple `==` check by default.
-/// - [areOwnPropsEqual] does a shallow Map equality check by default.
-/// - [areStatePropsEqual] does a shallow Map equality check by default.
-/// - [areMergedPropsEqual] does a shallow Map equality check by default.
+/// - [areStatesEqual] does an equality check using JS `===` (equivalent to [identical]) by default.
+/// - [areOwnPropsEqual] does a shallow Map equality check using JS `===` (equivalent to [identical]) by default.
+/// - [areStatePropsEqual] does a shallow Map equality check using JS `===` (equivalent to [identical]) by default.
+/// - [areMergedPropsEqual] does a shallow Map equality check using JS `===` (equivalent to [identical]) by default.
 ///
 /// - [context] can be utilized to provide a custom context object created with `createContext`.
 /// [context] is how you can utilize multiple stores. While supported, this is not recommended. :P
@@ -156,11 +156,6 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend
   bool pure = true,
   bool forwardRef = false,
 }) {
-  areStatesEqual ??= _defaultEquality;
-  areOwnPropsEqual ??= _shallowMapEquality;
-  areStatePropsEqual ??= _shallowMapEquality;
-  areMergedPropsEqual ??= _shallowMapEquality;
-
   UiFactory<TProps> wrapWithConnect(UiFactory<TProps> factory) {
     final dartComponentFactory = factory().componentFactory;
     final dartComponentClass = dartComponentFactory.type;
@@ -230,19 +225,31 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend
     bool handleAreMergedPropsEqual(JsMap jsNext, JsMap jsPrev) =>
         areMergedPropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev));
 
+    final connectOptions = JsConnectOptions(
+      forwardRef: forwardRef,
+      pure: pure,
+      context: context?.jsThis ?? JsReactRedux.ReactReduxContext,
+    );
+    // These can't be `null` in the JS object, so we conditionally define them
+    // so they won't exist in the object if we don't want to specify them.
+    if (areStatesEqual != null) {
+      connectOptions.areStatesEqual = allowInterop(handleAreStatesEqual);
+    }
+    if (areOwnPropsEqual != null) {
+      connectOptions.areOwnPropsEqual = allowInterop(handleAreOwnPropsEqual);
+    }
+    if (areStatePropsEqual != null) {
+      connectOptions.areStatePropsEqual = allowInterop(handleAreStatePropsEqual);
+    }
+    if (areMergedPropsEqual != null) {
+      connectOptions.areMergedPropsEqual = allowInterop(handleAreMergedPropsEqual);
+    }
+
     final hoc = mockableJsConnect(
       mapStateToProps != null ? allowInteropWithArgCount(handleMapStateToProps, 1) : mapStateToPropsWithOwnProps != null ? allowInteropWithArgCount(handleMapStateToPropsWithOwnProps, 2) : null,
       mapDispatchToProps != null ? allowInteropWithArgCount(handleMapDispatchToProps, 1) : mapDispatchToPropsWithOwnProps != null ? allowInteropWithArgCount(handleMapDispatchToPropsWithOwnProps, 2) : null,
       mergeProps != null ? allowInterop(handleMergeProps) : null,
-      JsConnectOptions(
-        areStatesEqual: allowInterop(handleAreStatesEqual),
-        areOwnPropsEqual: allowInterop(handleAreOwnPropsEqual),
-        areStatePropsEqual: allowInterop(handleAreStatePropsEqual),
-        areMergedPropsEqual: allowInterop(handleAreMergedPropsEqual),
-        forwardRef: forwardRef,
-        pure: pure,
-        context: context?.jsThis ?? JsReactRedux.ReactReduxContext,
-      ),
+      connectOptions,
     )(dartComponentClass);
 
     /// Use a Dart proxy instead of a JS one since we're treating it like a Dart component:
@@ -263,9 +270,6 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend
   return wrapWithConnect;
 }
 
-bool _defaultEquality(Object a, Object b) => a == b;
-bool _shallowMapEquality(Map a, Map b) => const MapEquality().equals(a, b);
-
 @JS('ReactRedux.connect')
 external ReactClass Function(ReactClass) _jsConnect(
     [
@@ -367,10 +371,15 @@ class ReactJsReactReduxComponentFactoryProxy extends ReactJsContextComponentFact
 }
 
 /// Converts a Redux.dart [Store] into a Javascript object formatted for consumption by react-redux.
-JsReactReduxStore _reduxifyStore(Store store){
+JsReactReduxStore _reduxifyStore(Store store) {
+  // Memoize this so that the same ReactInteropValue instances will be used
+  // for a given state, allowing JS `===` checks to not fail when the same
+  // state object is passed.
+  final memoizedWrapInteropValue = imemo1(wrapInteropValue);
+
   return JsReactReduxStore(
     getState: allowInterop(() {
-      return wrapInteropValue(store.state);
+      return memoizedWrapInteropValue(store.state);
     }),
     subscribe: allowInterop((cb) {
       return allowInterop(store.onChange.listen((_){cb();}).cancel);

lib/src/util/equality.dart

@@ -0,0 +1,35 @@
+/// Returns whether maps [a] and [b] have [identical] sets of values for the same keys.
+//
+// Ported from https://github.com/reduxjs/react-redux/blob/573db0bfc8d1d50fdb6e2a98bd8a7d4675fecf11/src/utils/shallowEqual.js
+//
+// The MIT License (MIT)
+//
+// Copyright (c) 2015-present Dan Abramov
+//
+// Permission is hereby granted, free of charge, to any person obtaining
+// a copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to
+// permit persons to whom the Software is furnished to do so, subject to
+// the following conditions:
+//
+// The above copyright notice and this permission notice shall be
+// included in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+bool areMapsShallowIdentical(Map a, Map b) {
+  if (identical(a, b)) return true;
+  if (a.length != b.length) return false;
+  for (final key in a.keys) {
+    if (!b.containsKey(key)) return false;
+    if (!identical(b[key], a[key])) return false;
+  }
+  return true;
+}

pubspec.yaml

@@ -17,6 +17,7 @@ dependencies:
   dart_style: ^1.2.5
   js: ^0.6.1+1
   logging: ">=0.11.3+2 <1.0.0"
+  memoize: ^2.0.0
   meta: ^1.1.6
   path: ^1.5.1
   react: ^5.3.0

test/over_react/component_declaration/component_base_test.dart

@@ -86,19 +86,35 @@ main() {
     });
   }
 
-  void _commonVariadicChildrenTests(UiProps builder) {
+  void _commonVariadicChildrenTests(UiProps builder, {bool alwaysExpectList = false}) {
     // There are different code paths for 0, 1, 2, 3, 4, 5, 6, and 6+ arguments.
     // Test all of them.
     group('a number of variadic children:', () {
-      test('0', () {
-        final instance = builder();
-        expect(getJsChildren(instance), isNull);
-      });
+      if (alwaysExpectList) {
+        test('0', () {
+          final instance = builder();
+          expect(getJsChildren(instance), []);
 
-      test('1', () {
-        final instance = builder(1);
-        expect(getJsChildren(instance), equals(1));
-      });
+          final instance2 = builder();
+          expect(getJsChildren(instance2), same(getJsChildren(instance)),
+              reason: 'zero arg children should always be the same List instance for perf reasons');
+        });
+
+        test('1', () {
+          final instance = builder(1);
+          expect(getJsChildren(instance), [1]);
+        });
+      } else {
+        test('0', () {
+          final instance = builder();
+          expect(getJsChildren(instance), isNull);
+        });
+
+        test('1', () {
+          final instance = builder(1);
+          expect(getJsChildren(instance), equals(1));
+        });
+      }
 
       const firstGeneralCaseVariadicChildCount = 2;
       const maxSupportedVariadicChildCount = 40;
@@ -137,9 +153,22 @@ main() {
         stopRecordingValidationWarnings();
       });
 
-      group('renders a DOM component with the correct children when', () {
-        _commonVariadicChildrenTests(Dom.div());
+      group('creates a ReactElement with the correct children:', () {
+        group('DomProps:', () {
+          _commonVariadicChildrenTests(Dom.div());
+        });
 
+        // Need to test these separately because they have different JS factory proxies
+        group('Dart Component:', () {
+          _commonVariadicChildrenTests(TestComponent());
+        });
+
+        group('Dart Component2:', () {
+          _commonVariadicChildrenTests(TestComponent2(), alwaysExpectList: true);
+        });
+      });
+
+      group('renders a DOM component with the correct children when', () {
         test('no children are passed in', () {
           var renderedNode = renderAndGetDom(Dom.div()());
 
@@ -201,8 +230,6 @@ main() {
       }, tags: 'ddc');
 
       group('renders a composite Dart component with the correct children when', () {
-        _commonVariadicChildrenTests(TestComponent());
-
         test('no children are passed in', () {
 
           var renderedInstance = render(TestComponent()());