CPLAT-11374: Add makeMapStateToProps/WithOwnProps and makeMapDispatchToProps/WithOwnProps by kealjones-wk · Pull Request #618 · Workiva/over_react

kealjones-wk · 2020-08-19T21:01:10Z

Motivation

React Redux allows you to return a function from mapStateToProps, allowing you to create a closure specific to that component instance, allowing you to set up a private copy of a memoized selector so that components don't invalidate each other's cache.

We should support this functionality so that consumers can follow the official React Redux tutorials for preventing unnecessary rerenders

Changes

Add makeMapStateToProps & makeMapStateToPropsWithOwnProps
Add makeMapDispatchToProps & makeMapDispatchToPropsWithOwnProps
Add tests for all added make factory functions
Update connect doc comments with make arguments
Update and Format over_react_redux_documentation.md with make arguments

Release Notes

Add makeMapStateToProps & makeMapStateToPropsWithOwnProps
Add makeMapDispatchToProps & makeMapDispatchToPropsWithOwnProps

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:
- 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

aviary-wf · 2020-08-19T21:03:28Z

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.

kealjones-wk · 2020-08-19T21:16:34Z

lib/src/over_react_redux/devtools/middleware.dart

    log.onRecord.listen((rec) {
      if (rec.level == Level.WARNING) {
-        window.console.warn('${log.name} [${rec.level.name}]: ${rec.message}');
+        callMethod(windowConsole, 'warn', ['${log.name} [${rec.level.name}]: ${rec.message}', if (rec.error != null) rec.error]);


changed to this to allow for "grouping" the error with the warning using console.warns varargs

kealjones-wk · 2020-08-19T21:20:14Z

lib/src/over_react_redux/over_react_redux.dart

      connectOptions.areMergedPropsEqual = allowInterop(handleAreMergedPropsEqual);
    }

+    dynamic interopMapStateToPropsHandler() {


typed return as dynamic in case we ever want to allow for the object based syntax

Can you make this a code comment?

greglittlefield-wf

Looks awesome, just some minor comments!

greglittlefield-wf · 2020-08-21T19:03:37Z

lib/src/over_react_redux/over_react_redux.dart

+        unwrapInteropValue(initialJsState),
+        jsPropsToTProps(initialJsOwnProps)
+      );
+      return allowInteropWithArgCount((jsState) {


Can we name this function so that it's easier to tell what this function is in the factory?

test/over_react_redux/connect_test.dart

greglittlefield-wf · 2020-08-21T19:08:37Z

doc/over_react_redux_documentation.md

-    ```dart
-    import 'package:over_react/over_react_redux.dart';
-    ```
+   ```dart


There's a lot of indentation changes in this file from 4 to 3 in some places and 2 in others.

Was there a reason to move away from 4? Can we at least make it consistent?

greglittlefield-wf · 2020-08-21T19:15:18Z

lib/src/over_react_redux/over_react_redux.dart

      connectOptions.areMergedPropsEqual = allowInterop(handleAreMergedPropsEqual);
    }

+    dynamic interopMapStateToPropsHandler() {


Can you make this a code comment?

greglittlefield-wf · 2020-08-21T19:17:22Z

lib/src/over_react_redux/over_react_redux.dart

 UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extends UiProps>({
  Map Function(TReduxState state) mapStateToProps,
  Map Function(TReduxState state, TProps ownProps) mapStateToPropsWithOwnProps,
+  Map Function(TReduxState state) Function(TReduxState initialState, TProps initialOwnProps) makeMapStateToProps,


Can we throw ArgumentError when a consumer specifies a "make" version as well as a non-"make" version?

Currently if they did both, one would silently be ignored

doc/over_react_redux_documentation.md

greglittlefield-wf · 2020-08-21T19:19:40Z

doc/over_react_redux_documentation.md

+  - Does a shallow Map equality check by default.
+  - Takes a function with the signature `(next: TProps, prev: TProps) => bool`.
+  - Can be passed in for small performance improvements. Two possible cases are to implement `deepEqual` or
+    `strictEqual`. > See: <https://react-redux.js.org/api/connect#aremergedpropsequal-next-object-prev-object-boolean>


This got messed up after formatting

Co-authored-by: Greg Littlefield <greg.littlefield@workiva.com>

…com/Workiva/over_react into CPLAT-11374-add-makeMapStateToProps

Co-authored-by: Greg Littlefield <greg.littlefield@workiva.com>

greglittlefield-wf · 2020-09-08T23:07:42Z

QA +1

greglittlefield-wf · 2020-09-08T23:34:02Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

kealjones-wk added 6 commits August 18, 2020 12:50

mapStateToProps factory functions

21e0a4e

update and add withOwnProps test

90d27cc

cleanup some minor error handling in middleware

db5220f

add makeMapDispatchToProps/WithOwnProps

80c15ac

update docs

4ad53e2

cleanup doc comment

0f4131e

rmconsole-wf added the Merge Requirements Unmet label Aug 19, 2020

revert example

61a8e50

kealjones-wk added 2 commits August 19, 2020 14:10

fix acidental breakage of docs

0138bbe

fix another format mistake

9f3c284

kealjones-wk commented Aug 19, 2020

View reviewed changes

kealjones-wk requested review from greglittlefield-wf and todbachman-wf August 19, 2020 22:28

greglittlefield-wf requested changes Aug 21, 2020

View reviewed changes

kealjones-wk and others added 8 commits August 31, 2020 15:39

Apply suggestions from code review

5bf27ce

Co-authored-by: Greg Littlefield <greg.littlefield@workiva.com>

address code review feedback

5508120

Merge branch 'CPLAT-11374-add-makeMapStateToProps' of https://github.…

6c0bf5c

…com/Workiva/over_react into CPLAT-11374-add-makeMapStateToProps

fix indenting

be41643

more indenting fixes

a7fb261

add argument error for multiple mapToProps args

df83f0e

fix test to prevent potential false positives

c4e9240

Apply suggestions from code review

b0a6317

Co-authored-by: Greg Littlefield <greg.littlefield@workiva.com>

aaronlademann-wf added Type: new feature area-redux labels Sep 3, 2020

kealjones-wk requested a review from greglittlefield-wf September 8, 2020 22:48

greglittlefield-wf approved these changes Sep 8, 2020

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Sep 8, 2020

greglittlefield-wf added the semver: Minor label Sep 8, 2020

rmconsole-wf added the RM Ready label Sep 8, 2020

rmconsole-wf approved these changes Sep 8, 2020

View reviewed changes

rmconsole4-wk merged commit 0779439 into master Sep 8, 2020

rmconsole4-wk deleted the CPLAT-11374-add-makeMapStateToProps branch September 8, 2020 23:34

Conversation

kealjones-wk commented Aug 19, 2020

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

Uh oh!

aviary-wf commented Aug 19, 2020

Security Insights

Action Items

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglittlefield-wf commented Sep 8, 2020

Uh oh!

greglittlefield-wf commented Sep 8, 2020

Uh oh!

rmconsole-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants