Merge pull request #730 from Workiva/fix-selector-equality

CPLAT-16997 Fix selector hooks redrawing when values haven't changed

Author: rmconsole6-wk

lib/src/component/pure_component_mixin.dart

@@ -27,8 +27,7 @@ import 'package:over_react/over_react.dart';
 ///   list of children and pass that in as shown in the example below:
 ///
 /// ```dart
-/// import 'package:memoize/memoize.dart';
-///
+/// // imemo1 is from the `memoize` package
 /// final getMemoizedChildren = imemo1((items) => items.map((item) {
 ///   return (Child()
 ///     ..key = item.id

lib/src/over_react_redux/hooks/use_selector.dart

@@ -19,8 +19,13 @@ import 'package:js/js.dart';
 import 'package:over_react/src/over_react_redux/over_react_redux.dart';
 import 'package:over_react/src/over_react_redux/hooks/use_dispatch.dart';
 import 'package:over_react/src/util/context.dart';
+import 'package:over_react/src/util/dart_value_wrapper.dart';
 import 'package:react/react_client/react_interop.dart' show ReactContext;
 
+// Notes:
+//
+// [1] This value could be either a raw value or a value wrapped in DartValueWrapper.
+
 // ----------------------------------------------------
 //  useSelector hook
 // ----------------------------------------------------
@@ -122,20 +127,26 @@ import 'package:react/react_client/react_interop.dart' show ReactContext;
 ///   parameterize it with.
 ///
 /// > Related: [useDispatch]
-TValue useSelector<TReduxState, TValue>(TValue Function(TReduxState state) selector, [
+TValue useSelector<TReduxState, TValue>(
+  TValue Function(TReduxState state) selector, [
   bool Function(TValue tNextValue, TValue tPrevValue) equalityFn,
 ]) {
-  ReactInteropValue jsSelector(ReactInteropValue jsState) => wrapInteropValue(selector(unwrapInteropValue(jsState)));
+  Object /*[1]*/ jsSelector(Object /*[1]*/ jsState) =>
+      DartValueWrapper.wrapIfNeeded(selector(DartValueWrapper.unwrapIfNeeded(jsState)));
   _JsReduxStateEqualityFn jsEqualityFn = equalityFn == null
       ? null
       : allowInterop((nextJsValue, prevJsValue) =>
-          equalityFn(unwrapInteropValue(nextJsValue), unwrapInteropValue(prevJsValue)));
+          equalityFn(DartValueWrapper.unwrapIfNeeded(nextJsValue), DartValueWrapper.unwrapIfNeeded(prevJsValue)));
 
-  return unwrapInteropValue<TValue>(_jsUseSelector(allowInterop(jsSelector), jsEqualityFn));
+  if (jsEqualityFn == null) {
+    return DartValueWrapper.unwrapIfNeeded(_jsUseSelector(allowInterop(jsSelector)));
+  } else {
+    return DartValueWrapper.unwrapIfNeeded(_jsUseSelector(allowInterop(jsSelector), jsEqualityFn));
+  }
 }
 
 @JS('ReactRedux.useSelector')
-external ReactInteropValue _jsUseSelector(_JsSelectorFn selector, [_JsReduxStateEqualityFn equalityFn]);
+external Object /*[1]*/ _jsUseSelector(_JsSelectorFn selector, [_JsReduxStateEqualityFn equalityFn]);
 
 // ----------------------------------------------------
 //  createSelectorHook
@@ -197,16 +208,22 @@ external ReactInteropValue _jsUseSelector(_JsSelectorFn selector, [_JsReduxState
 /// ```
 _SelectorFnHook<TReduxState> createSelectorHook<TReduxState>([Context context]) {
   final jsHook = _jsCreateSelectorHook(context?.jsThis ?? JsReactRedux.ReactReduxContext);
-  TValue dartHook<TValue>(TValue Function(TReduxState state) selector, [
+  TValue dartHook<TValue>(
+    TValue Function(TReduxState state) selector, [
     bool Function(TValue tNextValue, TValue tPrevValue) equalityFn,
   ]) {
-    ReactInteropValue jsSelector(ReactInteropValue jsState) => wrapInteropValue(selector(unwrapInteropValue(jsState)));
+    Object /*[1]*/ jsSelector(Object /*[1]*/ jsState) =>
+        DartValueWrapper.wrapIfNeeded(selector(DartValueWrapper.unwrapIfNeeded(jsState)));
     _JsReduxStateEqualityFn jsEqualityFn = equalityFn == null
         ? null
         : allowInterop((nextJsValue, prevJsValue) =>
-            equalityFn(unwrapInteropValue(nextJsValue), unwrapInteropValue(prevJsValue)));
+            equalityFn(DartValueWrapper.unwrapIfNeeded(nextJsValue), DartValueWrapper.unwrapIfNeeded(prevJsValue)));
 
-    return unwrapInteropValue<TValue>(jsHook(allowInterop(jsSelector), jsEqualityFn));
+    if (jsEqualityFn == null) {
+      return DartValueWrapper.unwrapIfNeeded(jsHook(allowInterop(jsSelector)));
+    } else {
+      return DartValueWrapper.unwrapIfNeeded(jsHook(allowInterop(jsSelector), jsEqualityFn));
+    }
   }
 
   return dartHook;
@@ -219,9 +236,9 @@ external _JsSelectorFnHook _jsCreateSelectorHook(ReactContext context);
 //  Typedefs
 // ----------------------------------------------------
 
-typedef _JsSelectorFn = ReactInteropValue Function(ReactInteropValue jsState);
-typedef _JsReduxStateEqualityFn = bool Function(ReactInteropValue nextJsValue, ReactInteropValue prevJsValue);
-typedef _JsSelectorFnHook = ReactInteropValue Function(_JsSelectorFn selector, [_JsReduxStateEqualityFn equalityFn]);
+typedef _JsSelectorFn = Object /*[1]*/ Function(Object /*[1]*/ jsState);
+typedef _JsReduxStateEqualityFn = bool Function(Object /*[1]*/ nextJsValue, Object /*[1]*/ prevJsValue);
+typedef _JsSelectorFnHook = Object /*[1]*/ Function(_JsSelectorFn selector, [_JsReduxStateEqualityFn equalityFn]);
 typedef _SelectorFnHook<TReduxState> = TValue Function<TValue>(
   TValue Function(TReduxState state) selector, [
   bool Function(TValue tNextValue, TValue tPrevValue) equalityFn,

lib/src/over_react_redux/over_react_redux.dart

@@ -20,13 +20,13 @@ import 'dart:html';
 import 'dart:js_util' as js_util;
 
 import 'package:js/js.dart';
-import 'package:memoize/memoize.dart';
 import 'package:meta/meta.dart';
 import 'package:over_react/component_base.dart';
 import 'package:over_react/src/component_declaration/annotations.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:over_react/src/util/context.dart';
+import 'package:over_react/src/util/dart_value_wrapper.dart';
 import 'package:over_react/src/util/equality.dart';
 import 'package:react/react_client.dart';
 import 'package:react/react_client/js_backed_map.dart';
@@ -35,6 +35,10 @@ import 'package:redux/redux.dart';
 
 part 'over_react_redux.over_react.g.dart';
 
+// Notes:
+//
+// [1] This value could be either a raw value or a value wrapped in DartValueWrapper.
+
 /// This class is present:
 ///
 /// 1. to allow for consumers which have used the --backwards-compat flag with over_react_codemod to statically analyze:
@@ -200,47 +204,47 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend
       return interopFunction;
     }
 
-    JsMap handleMapStateToProps(ReactInteropValue jsState) {
+    JsMap handleMapStateToProps(Object /*[1]*/ jsState) {
       return jsMapFromProps(
         mapStateToProps(
-          unwrapInteropValue(jsState),
+          DartValueWrapper.unwrapIfNeeded(jsState),
         ),
       );
     }
 
-    JsMap handleMapStateToPropsWithOwnProps(ReactInteropValue jsState, JsMap jsOwnProps) {
+    JsMap handleMapStateToPropsWithOwnProps(Object /*[1]*/ jsState, JsMap jsOwnProps) {
       return jsMapFromProps(
         mapStateToPropsWithOwnProps(
-          unwrapInteropValue(jsState),
+          DartValueWrapper.unwrapIfNeeded(jsState),
           jsPropsToTProps(jsOwnProps),
         ),
       );
     }
 
-    JsMap Function(ReactInteropValue jsState) handleMakeMapStateToProps(ReactInteropValue initialJsState, JsMap initialJsOwnProps) {
+    JsMap Function(Object /*[1]*/ jsState) handleMakeMapStateToProps(Object /*[1]*/ initialJsState, JsMap initialJsOwnProps) {
       var mapToFactory = makeMapStateToProps(
-        unwrapInteropValue(initialJsState),
+        DartValueWrapper.unwrapIfNeeded(initialJsState),
         jsPropsToTProps(initialJsOwnProps)
       );
-      JsMap handleMakeMapStateToPropsFactory(ReactInteropValue jsState) {
+      JsMap handleMakeMapStateToPropsFactory(Object /*[1]*/ jsState) {
         return jsMapFromProps(
           mapToFactory(
-            unwrapInteropValue(jsState),
+            DartValueWrapper.unwrapIfNeeded(jsState),
           ),
         );
       }
       return allowInteropWithArgCount(handleMakeMapStateToPropsFactory, 1);
     }
 
-    JsMap Function(ReactInteropValue jsState, JsMap jsOwnProps) handleMakeMapStateToPropsWithOwnProps(ReactInteropValue initialJsState, JsMap initialJsOwnProps) {
+    JsMap Function(Object /*[1]*/ jsState, JsMap jsOwnProps) handleMakeMapStateToPropsWithOwnProps(Object /*[1]*/ initialJsState, JsMap initialJsOwnProps) {
       var mapToFactory = makeMapStateToPropsWithOwnProps(
-        unwrapInteropValue(initialJsState),
+        DartValueWrapper.unwrapIfNeeded(initialJsState),
         jsPropsToTProps(initialJsOwnProps)
       );
-      JsMap handleMakeMapStateToPropsWithOwnPropsFactory(ReactInteropValue jsState, JsMap jsOwnProps) {
+      JsMap handleMakeMapStateToPropsWithOwnPropsFactory(Object /*[1]*/ jsState, JsMap jsOwnProps) {
         return jsMapFromProps(
           mapToFactory(
-            unwrapInteropValue(jsState),
+            DartValueWrapper.unwrapIfNeeded(jsState),
             jsPropsToTProps(jsOwnProps),
           ),
         );
@@ -304,8 +308,8 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend
       );
     }
 
-    bool handleAreStatesEqual(ReactInteropValue jsNext, ReactInteropValue jsPrev) =>
-        areStatesEqual(unwrapInteropValue(jsNext), unwrapInteropValue(jsPrev));
+    bool handleAreStatesEqual(Object /*[1]*/ jsNext, Object /*[1]*/ jsPrev) =>
+        areStatesEqual(DartValueWrapper.unwrapIfNeeded(jsNext), DartValueWrapper.unwrapIfNeeded(jsPrev));
 
     bool handleAreOwnPropsEqual(JsMap jsNext, JsMap jsPrev) =>
         areOwnPropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev));
@@ -497,14 +501,9 @@ class ReactJsReactReduxComponentFactoryProxy extends ReactJsContextComponentFact
 
 /// Converts a Redux.dart [Store] into a Javascript object formatted for consumption by react-redux.
 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 memoizedWrapInteropValue(store.state);
+      return DartValueWrapper.wrapIfNeeded(store.state);
     }),
     subscribe: allowInterop((cb) {
       return allowInterop(store.onChange.listen((_){cb();}).cancel);
@@ -527,7 +526,7 @@ class JsReactReduxStore {
   external Store get dartStore;
 
   external factory JsReactReduxStore({
-    ReactInteropValue Function() getState,
+    Object /*[1]*/ Function() getState,
     Dispatcher dispatch,
     Function Function(Function) subscribe,
     Store dartStore,
@@ -538,22 +537,22 @@ class JsReactReduxStore {
 @JS()
 @anonymous
 class JsConnectOptions {
-    external set areStatesEqual(bool Function(ReactInteropValue, ReactInteropValue) value);
+    external set areStatesEqual(bool Function(Object /*[1]*/, Object /*[1]*/) value);
     external set areOwnPropsEqual(bool Function(JsMap, JsMap) value);
     external set areStatePropsEqual(bool Function(JsMap, JsMap) value);
     external set areMergedPropsEqual(bool Function(JsMap, JsMap) value);
     external set forwardRef(bool value);
     external set pure(bool value);
     external set context(ReactContext value);
-    external bool Function(ReactInteropValue, ReactInteropValue) get areStatesEqual;
+    external bool Function(Object /*[1]*/, Object /*[1]*/) get areStatesEqual;
     external bool Function(JsMap, JsMap) get areOwnPropsEqual;
     external bool Function(JsMap, JsMap) get areStatePropsEqual;
     external bool Function(JsMap, JsMap) get areMergedPropsEqual;
     external bool get forwardRef;
     external bool get pure;
     external ReactContext get context;
   external factory JsConnectOptions({
-    bool Function(ReactInteropValue, ReactInteropValue) areStatesEqual,
+    bool Function(Object /*[1]*/, Object /*[1]*/) areStatesEqual,
     bool Function(JsMap, JsMap) areOwnPropsEqual,
     bool Function(JsMap, JsMap) areStatePropsEqual,
     bool Function(JsMap, JsMap) areMergedPropsEqual,
@@ -563,17 +562,3 @@ class JsConnectOptions {
   });
 }
 
-/// A wrapper class that prevents dart2js from jsifying [value].
-class ReactInteropValue {
-  dynamic value;
-}
-
-/// A helper function that retrieves the [value] from a [ReactInteropValue].
-T unwrapInteropValue<T>(ReactInteropValue value) {
-  return value.value as T;
-}
-
-/// A helper function that wraps a [value] in a [ReactInteropValue].
-ReactInteropValue wrapInteropValue(dynamic value) {
-  return ReactInteropValue()..value = value;
-}

lib/src/util/dart_value_wrapper.dart

@@ -0,0 +1,43 @@
+// Copyright 2022 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 'dart:js';
+
+// Taken from react-dart
+/// A wrapper around a value that can't be stored in its raw form
+/// within a JS object (e.g., a Dart function).
+class DartValueWrapper {
+  final Object value;
+
+  const DartValueWrapper(this.value);
+
+  static final _functionWrapperCache = Expando<DartValueWrapper>('_functionWrapperCache');
+
+  static Object wrapIfNeeded(Object value) {
+    // This case should be fairly uncommon, since functions usually aren't used as
+    // a Redux store's state or the result of a connect or selector hook selector.
+    if (value is Function && !identical(allowInterop(value), value)) {
+      // Reuse wrappers for the same function so that they're identical to the JS.
+      return _functionWrapperCache[value] ??= DartValueWrapper(value);
+    }
+    return value;
+  }
+
+  static T unwrapIfNeeded<T>(Object value) {
+    if (value is DartValueWrapper) {
+      return value.value as T;
+    }
+    return value as T;
+  }
+}

pubspec.yaml

@@ -17,7 +17,6 @@ dependencies:
   dart_style: ^1.2.5
   js: ^0.6.1+1
   logging: '>=0.11.3+2 <2.0.0'
-  memoize: ^2.0.0
   meta: ">=1.1.6 <1.7.0" # Workaround to avoid https://github.com/dart-lang/sdk/issues/46142
   path: ^1.5.1
   react: ^6.1.4