UIP-1897: Add support for traversing wrapper components to getProps #48

jacehensley-wf · 2017-02-14T20:02:10Z

Ultimate problem:

getProps did not support traversing "Wrapper" components.

How it was fixed:

Add param traverseWrappers to getProps
- When true will traverse all "Wrapper" components until it finds the first nested non-"Wrapper" component.
- This is supported for ReactElements and Dart ReactComponents.

Also done:

Bump min SDK restraint to 1.21.1

Testing suggestions:

Verify tests pass
Verify tests cover all cases.

Potential areas of regression:

None expected, only new logic is added.

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf @kaanaksoy-wk

UIP-1897

aviary2-wf · 2017-02-14T20:02:14Z

Raven

Number of Findings: 0

UIP-1897

codecov-io · 2017-02-14T20:31:28Z

Codecov Report

Merging #48 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   97.65%   97.67%   +0.02%     
==========================================
  Files          28       28              
  Lines        1318     1329      +11     
==========================================
+ Hits         1287     1298      +11     
  Misses         31       31

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6543e36...f1e528c. Read the comment docs.

UIP-1897 - Get the component type meta of Dart ReactComponents by using the JS constructor as the type alias.

aaronlademann-wf

Looks great @jacehensley-wf! Just a few typos / doc comments update.

aaronlademann-wf · 2017-02-15T19:46:23Z

lib/src/util/react_wrappers.dart

@@ -108,10 +109,34 @@ Expando<UnmodifiableMapView> _elementPropsCache = new Expando('_elementPropsCach
 /// For a JS component, this returns the result of [getJsProps] in an unmodifiable Map view.
 ///
 /// Throws if [instance] is not a valid [ReactElement] or composite [ReactComponent] .
-Map getProps(/* ReactElement|ReactComponent */ instance) {
+Map getProps(/* ReactElement|ReactComponent */ instance, {bool traverseWrappers: false}) {


@jacehensley-wf the doc comment should be updated to explain the use case of setting traverseWrappers to true.

aaronlademann-wf · 2017-02-15T19:48:21Z

test/over_react/util/react_wrappers_test.dart

@@ -574,6 +576,264 @@ main() {
        }));
      });

+      group('traverses children of Wrapper components', () {
+        group('and retruns props for a', () {


s/retruns/returns

aaronlademann-wf · 2017-02-15T19:49:10Z

test/over_react/util/react_wrappers_test.dart

+              }));
+            });
+
+            test('except when traversWrappers is false', () {


s/traversWrappers/traverseWrappers

aaronlademann-wf · 2017-02-15T19:49:41Z

test/over_react/util/react_wrappers_test.dart

+              }));
+            });
+
+            test('except when traversWrappers is false', () {


s/traversWrappers/traverseWrappers

aaronlademann-wf · 2017-02-15T19:49:58Z

test/over_react/util/react_wrappers_test.dart

+              }));
+            });
+
+            test('except when traversWrappers is false', () {


s/traversWrappers/traverseWrappers

aaronlademann-wf · 2017-02-15T19:50:23Z

test/over_react/util/react_wrappers_test.dart

+              }));
+            });
+
+            test('except when traverseWrapers is false', () {


s/traverseWrapers/traverseWrappers

greglittlefield-wf · 2017-02-15T19:54:44Z

lib/src/util/react_wrappers.dart

+      ComponentTypeMeta instanceTypeMeta;
+
+      if (isCompositeComponent && isDartComponent(instance)) {
+        var reactClassType = getProperty(getDartComponent(instance).jsThis, 'constructor');


Sorry I wasn't more clear on what I meant here; I was suggesting something like:

ReactClass type = getProperty(getDartComponent(instance).jsThis, 'constructor'); instanceTypeMeta = getComponentTypeMeta(type);

UIP-1897

jacehensley-wf · 2017-02-15T22:49:53Z

@greglittlefield-wf @aaronlademann-wf Feedback has been addressed.

aaronlademann-wf · 2017-02-15T22:57:33Z

+1

clairesarsam-wf · 2017-02-16T20:25:49Z

+1

greglittlefield-wf · 2017-02-16T23:21:38Z

test/over_react/util/react_wrappers_test.dart

+                }, testChildren)
+              ));
+
+              expect(getProps(renderedInstance), isNot({


Would probably be better to verify explicitly that the OneLevelWrapper's props are returned.

(Same goes for other analogous tests)

Not sure why this is still here. This has been addressed.

greglittlefield-wf · 2017-02-16T23:23:48Z

test/over_react/util/react_wrappers_test.dart

@@ -574,6 +576,264 @@ main() {
        }));
      });

+      group('traverses children of Wrapper components', () {
+        group('and returns props for a', () {
+          group('composite JS ReactComponent', () {


Seems like composite JS ReactElements aren't tested

greglittlefield-wf · 2017-02-16T23:27:38Z

test/over_react/util/react_wrappers_test.dart

+          });
+        });
+      });
+


It'd probably be good to also test that getProps w/ traverseWrappers: true doesn't have any issues when passed a non-wrapper component.

greglittlefield-wf · 2017-02-16T23:28:14Z

A few comments around tests, but otherwise looks great

UIP-1897

jacehensley-wf · 2017-02-16T23:58:12Z

@greglittlefield-wf feedback has been addressed

greglittlefield-wf · 2017-02-17T00:13:47Z

+1

leviwith-wf · 2017-02-20T22:38:53Z

QA Resource Approval: +10

Merging.

Add support for traversing wrapper components to getProps

a16104e

UIP-1897

jacehensley-wf added area-utils State: needs review Type: new feature labels Feb 14, 2017

jacehensley-wf added 4 commits February 14, 2017 13:11

Bump min SDK restraint to 1.21.1

c7f43b7

UIP-1897

Use returnNormally instead of isNot(throws)

b8e2c49

UIP-1897

Fix zonedExpect for test versions where that expect isn't void

74e58d4

UIP-1897

Fix brittle matcher test by ignoring all whitespace in failure message

017b4cb

UIP-1897

jacehensley-wf added State: in progress and removed State: needs review labels Feb 14, 2017

Fix dart2js test failures

ea025bc

UIP-1897 - Get the component type meta of Dart ReactComponents by using the JS constructor as the type alias.

jacehensley-wf added State: needs review and removed State: in progress labels Feb 14, 2017

aaronlademann-wf requested changes Feb 15, 2017

View reviewed changes

greglittlefield-wf reviewed Feb 15, 2017

View reviewed changes

jacehensley-wf added 2 commits February 15, 2017 15:42

Address feedback

4284530

UIP-1897

Fix typo

b271ef4

UIP-1897

aaronlademann-wf approved these changes Feb 15, 2017

View reviewed changes

rmconsole-wf added the Merge Requirements Met label Feb 15, 2017

aaronlademann-wf added State: ready for QA and removed Merge Requirements Met State: needs review labels Feb 15, 2017

rmconsole-wf added the Merge Requirements Met label Feb 16, 2017

greglittlefield-wf reviewed Feb 16, 2017

View reviewed changes

greglittlefield-wf removed the State: ready for QA label Feb 16, 2017

jacehensley-wf added 2 commits February 16, 2017 16:52

Address CR feedback

f1f97c3

UIP-1897

Remove print statements

f1e528c

UIP-1897

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Feb 16, 2017

greglittlefield-wf added the State: ready for QA label Feb 17, 2017

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Feb 17, 2017

leviwith-wf merged commit a0af902 into Workiva:master Feb 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIP-1897: Add support for traversing wrapper components to getProps #48

UIP-1897: Add support for traversing wrapper components to getProps #48

jacehensley-wf commented Feb 14, 2017 •

edited

Loading

aviary2-wf commented Feb 14, 2017

codecov-io commented Feb 14, 2017 •

edited

Loading

aaronlademann-wf left a comment

aaronlademann-wf Feb 15, 2017

aaronlademann-wf Feb 15, 2017

aaronlademann-wf Feb 15, 2017

aaronlademann-wf Feb 15, 2017

aaronlademann-wf Feb 15, 2017

aaronlademann-wf Feb 15, 2017

greglittlefield-wf Feb 15, 2017

jacehensley-wf commented Feb 15, 2017

aaronlademann-wf commented Feb 15, 2017

clairesarsam-wf commented Feb 16, 2017

greglittlefield-wf Feb 16, 2017

greglittlefield-wf Feb 16, 2017

jacehensley-wf Feb 16, 2017

greglittlefield-wf Feb 16, 2017

greglittlefield-wf Feb 16, 2017

jacehensley-wf Feb 16, 2017

greglittlefield-wf commented Feb 16, 2017

jacehensley-wf commented Feb 16, 2017

greglittlefield-wf commented Feb 17, 2017

leviwith-wf commented Feb 20, 2017

UIP-1897: Add support for traversing wrapper components to getProps #48

UIP-1897: Add support for traversing wrapper components to getProps #48

Conversation

jacehensley-wf commented Feb 14, 2017 • edited Loading

Ultimate problem:

How it was fixed:

Also done:

Testing suggestions:

Potential areas of regression:

aviary2-wf commented Feb 14, 2017

Raven

codecov-io commented Feb 14, 2017 • edited Loading

Codecov Report

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacehensley-wf commented Feb 15, 2017

aaronlademann-wf commented Feb 15, 2017

clairesarsam-wf commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf commented Feb 16, 2017

jacehensley-wf commented Feb 16, 2017

greglittlefield-wf commented Feb 17, 2017

leviwith-wf commented Feb 20, 2017

jacehensley-wf commented Feb 14, 2017 •

edited

Loading

codecov-io commented Feb 14, 2017 •

edited

Loading