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

greglittlefield-wf · 2022-02-03T18:11:23Z

Motivation

The useSelector hook and hooks created by createSelectorHook allow consumers to subscribe to store updates and rerender the component only when a value returned from the selector function changes.

However, the Dart versions of these APIs were always rerendering, even when the selected value didn't change.

This was caused by two issues:

We were always wrapping the selected value in a new ReactInteropValue object, which meant that when you didn't provide a custom equality function, the default behavior on the JS side of comparing the value using === would always think the value changed
We were always passing null into the hook as an equality function as opposed to conditionally calling it, which was causing null errors in React Redux JS that were getting ignored

Changes

Instead of always wrapping in an interop value, conditionally wrap only when necessary (Dart functions), just like we do in react-dart's JsBackedMap
Conditionally pass the equality function instead of always passing in null
Add regression tests to ensure renders only occur when we expect them to, for both hooks and connect
Add test coverage for values of different types, which also tests conditional wrapping of values
Add useSelector example

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
  - Serve the examples locally webdev serve
  - Go to http://localhost:8080/over_react_redux/examples/simple/
  - Start a React Dev Tools profile, click one of the BigCounter buttons, and stop the profile
  - Verify that only the "BigCounter" components rerender, and not the "Counter" or "HookCounter" components
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Client Platform member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

…values

…tup checks

…t casts, tidy up

aviary-wf · 2022-02-03T18:12:44Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

greglittlefield-wf · 2022-02-03T18:37:57Z

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


I changed this to work around dependency_validator flagging imports in comments

greglittlefield-wf · 2022-02-03T18:39:01Z

lib/src/over_react_redux/over_react_redux.dart

@@ -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.


Since we pass through the value directly (and memoize it in the cases we wrap it in DartValueWrapper.wrapIfNeeded), this is no longer necessary

greglittlefield-wf · 2022-02-03T18:40:48Z

test/over_react_redux/hooks/use_selector_test.dart

-      });
-    });
-  });
-}


These tests have moved to store_bindings_tests.dart. I started by refactoring them to use custom components so they could be run with multiple types of selected values, and they eventually morphed into tests that could also be shared with connect, thus the new file name.

greglittlefield-wf · 2022-02-03T18:43:15Z

test/over_react_redux/store_bindings_tests.dart

@@ -0,0 +1,420 @@
+// Copyright 2022 Workiva Inc.


I'm open to better name suggestions for this file

aaronlademann-wf

QA +1

joebingham-wk

+1, looks great! Those are some dope tests 🎉

joebingham-wk · 2022-02-03T19:52:29Z

test/over_react_redux/store_bindings_tests.dart

+      if (allValues.toSet().length != allValues.length) {
+        throw ArgumentError('initialValue and updatedValues must all be unique.');
+      }


Oohh this is clever. TIL

KealJones

+1

greglittlefield-wf · 2022-02-03T20:46:05Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

greglittlefield-wf added 17 commits February 2, 2022 12:16

Fix selectors always returning new objects by conditionally wrapping …

64d087e

…values

Reuse wrapped functions

c01d310

Add regression tests

2c68978

Fix missing arg causing swallowed RTEs

5e12f20

Fix missing arg causing swallowed RTEs for createSelectorHook

4dfb35d

Add some shared tests for different selected value types

656cfca

Add test coverage for createSelector, add null case and additional se…

df95525

…tup checks

Use typed props

d472036

Fix typo

498c594

Update typing from dynamic to Object to prevent unintentional implici…

2e74a56

…t casts, tidy up

Revert changes in old tests

4202bc5

Remove unused dependency

f4f3ae4

Add more test coverage

5973053

Cleanup

2880847

Add test coverage for value identity

7cee6c0

Add additional test coverage for connect

abe3425

Add comment

e7cabbd

rmconsole-wf added the Merge Requirements Unmet label Feb 3, 2022

greglittlefield-wf added 2 commits February 3, 2022 11:30

Add useSelector example

b199497

Update doc comment to not get flagged by dependency_validator

b55e993

greglittlefield-wf commented Feb 3, 2022

View reviewed changes

Fix copyright headers

cb65a0e

greglittlefield-wf commented Feb 3, 2022

View reviewed changes

greglittlefield-wf requested review from sydneesampson-wk, sydneyjodon-wk, kealjones-wk and aaronlademann-wf February 3, 2022 18:44

greglittlefield-wf requested review from joebingham-wk and brianphillips-wk February 3, 2022 18:44

rmconsole6-wk changed the title ~~Fix selector hooks redrawing when values haven't changed~~ CPLAT-16997 Fix selector hooks redrawing when values haven't changed Feb 3, 2022

aaronlademann-wf approved these changes Feb 3, 2022

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Feb 3, 2022

joebingham-wk approved these changes Feb 3, 2022

View reviewed changes

KealJones approved these changes Feb 3, 2022

View reviewed changes

rmconsole-wf added the RM Ready label Feb 3, 2022

rmconsole-wf approved these changes Feb 3, 2022

View reviewed changes

rmconsole6-wk merged commit 42df42e into master Feb 3, 2022

rmconsole6-wk deleted the fix-selector-equality branch February 3, 2022 20:46

This was referenced Feb 3, 2022

RM-127138 RM-127137 Release over_react 4.2.7 #731

Merged

Consumer testing for https://github.com/Workiva/over_react/pull/731 Workiva/over_react_test#132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

greglittlefield-wf commented Feb 3, 2022 •

edited

Loading

aviary-wf commented Feb 3, 2022

greglittlefield-wf Feb 3, 2022

greglittlefield-wf Feb 3, 2022

greglittlefield-wf Feb 3, 2022

greglittlefield-wf Feb 3, 2022

aaronlademann-wf left a comment •

edited

Loading

joebingham-wk left a comment

joebingham-wk Feb 3, 2022

KealJones left a comment

greglittlefield-wf commented Feb 3, 2022

rmconsole-wf left a comment

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

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

Conversation

greglittlefield-wf commented Feb 3, 2022 • edited Loading

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

aviary-wf commented Feb 3, 2022

Security Insights

Action Items

greglittlefield-wf Feb 3, 2022

Choose a reason for hiding this comment

greglittlefield-wf Feb 3, 2022

Choose a reason for hiding this comment

greglittlefield-wf Feb 3, 2022

Choose a reason for hiding this comment

greglittlefield-wf Feb 3, 2022

Choose a reason for hiding this comment

aaronlademann-wf left a comment • edited Loading

Choose a reason for hiding this comment

joebingham-wk left a comment

Choose a reason for hiding this comment

joebingham-wk Feb 3, 2022

Choose a reason for hiding this comment

KealJones left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Feb 3, 2022

rmconsole-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Feb 3, 2022 •

edited

Loading

aaronlademann-wf left a comment •

edited

Loading