CPLAT-12194 Enable functional components to use consumed props #633

joebingham-wk · 2020-10-09T16:47:12Z

Motivation

In class based components, we have the API's addUnconsumedProps to facilitate passing along props more easily. We want the same behavior in functional components.

Changes

Create a new class (PropsInstanceMeta) that uses a mixin on PropsMetaCollection to manage consumed props operations (e0cde45)
Add a property onto UiProps called $meta typed as PropsInstanceMeta (e0cde45)
Update the builder to override the field on Component2 / functional component props instances (8855f7e)
Add a mixin (UiPropsMeta) on UiProps that can be used to access $meta, along with adding APIs to derive consumed props (e0cde45)
Add examples (51b5466)

STILL LEFT TO DO:

Receive feedback on the approach thus far
Add doc comments / documentation
Add tests

Release Notes

Add APIs for using consumed props in functional components

Review

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

Please review:
@aaronlademann-wf @greglittlefield-wf @sydneyjodon-wk

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-10-09T16:48:58Z

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.

example/builder/src/new_class_consumed_props.dart

greglittlefield-wf · 2020-10-09T19:48:34Z

lib/src/builder/codegen/typed_map_impl_generator.dart

+        allPropsMixins = declaration.props.either.nodeHelper.mixins
+            ?.map<Identifier>((mixin) => mixin.name)
+            ?.toList(),


I think this implementation might not be correct when using the shorthand syntax, since .nodeHelper.mixins on a mixin will be empty.

For class components, we're using allPropsMixins, which does account for that case. Could we share that same implementation between ClassComponentDeclaration and PropsMapViewOrFunctionComponentDeclaration so that we can use it here as well?

Definitely, good catch. Updated in ea0e4aa!

greglittlefield-wf · 2020-10-09T19:49:53Z

lib/src/builder/codegen/util.dart

+  buffer
+    ..writeln()
+    ..writeln('  @override')
+    ..writeln('  $classType get $fieldName => ${isConst ? 'const' : ''} $classType({');


Why is the instance conditionally const? Can't it always be const?

I might be missing something, but not with the current implementation of InstancePropsMeta because it has instance fields. If I change that to not keep that state though, then that will clean this up as well!

Updated this in ea0e4aa!

greglittlefield-wf

Noticed a couple things but this generally looks great!

example/builder/src/new_class_consumed_props.dart

lib/src/component_declaration/builder_helpers.dart

greglittlefield-wf · 2020-10-14T22:41:20Z

lib/src/util/map_util.dart

@@ -75,6 +76,9 @@ Map getPropsToForward(Map props, {
 ///
 /// Based upon configuration, the function will overlook [props] that are not
 /// meant to be passed on, such as non-DOM props or specified values.
+///
+/// DEPRECATED: Use [forwardUnconsumedPropsV2] instead.
+@Deprecated('This implementation does not filter DOM props correctly. Use forwardUnconsumedPropsV2 instead.')


greglittlefield-wf · 2020-10-14T22:45:50Z

lib/src/util/react_util.dart

@@ -40,6 +40,8 @@ class UiPropsMapView extends MapView
  bool get $isClassGenerated =>
      throw UnimplementedError('@PropsMixin instances do not implement \$isClassGenerated');

+  PropsMetaCollection get $meta => throw UnimplementedError('@PropsMixin instances do not implement instance meta');


Unrelated to this PR, but we should really deprecate the UiPropsMapView class... It's made obsolete by the new boilerplate for props map views, which are real UiProps classes

:noice: Updated in 88e20b7!

lib/src/component_declaration/builder_helpers.dart

joebingham-wk · 2020-10-15T16:24:41Z

lib/src/component_declaration/component_base.dart

+  ///
+  /// This is useful in conjunction with [UiProps]'s `addUnconsumedProps`,
+  /// which expects [PropsMeta] to be provided in a [List].
+  List<PropsMeta> inList() => [this];


Added this because of this

joebingham-wk · 2020-10-15T16:33:17Z

test/over_react/util/map_util_test.dart

-          'prop 5': 'my prop #5',
-          'prop 6': 'my prop #6',
-        }, keySetsToOmit: [], propsToUpdate: actual);
+        test('when keySetsToOmit is null', () {


This is the only new test in this function

joebingham-wk · 2020-10-15T16:33:47Z

test/over_react/util/map_util_test.dart

-            'ref': 'my ref',
-            'other prop': 'my other prop'
-          };
+    void commonPropsForwardingUtilTests(ForwardUnconsumedPropsFunction functionToTest) {


These tests (in commonPropsForwardingUtilTests) were all here originally - except for one, which is noted below.

greglittlefield-wf

Found a couple things, but this is looking great; should be my final pass!

lib/src/component_declaration/builder_helpers.dart

...component_declaration/builder_integration_tests/new_boilerplate/function_component_test.dart

example/builder/src/functional_consumed_props.dart

lib/src/component_declaration/builder_helpers.dart

lib/src/util/map_util.dart

lib/src/component_declaration/builder_helpers.dart

greglittlefield-wf

Just one more small comment

lib/src/component_declaration/builder_helpers.dart

lib/src/util/react_util.dart

lib/src/component/strictmode_component.dart

lib/src/util/react_util.dart

greglittlefield-wf · 2020-10-23T20:38:55Z

QA +1

aaronlademann-wf

+1

This looks awesome @joebingham-wk !!!

joebingham-wk · 2020-10-26T05:53:19Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

Update builder

8855f7e

rmconsole-wf added the Merge Requirements Unmet label Oct 9, 2020

joebingham-wk requested review from aaronlademann-wf, greglittlefield-wf and sydneyjodon-wk October 9, 2020 17:03

joebingham-wk added 3 commits October 9, 2020 10:28

Add new APIs

e0cde45

Add new examples

51b5466

Add all other generated code

7c252f5

joebingham-wk force-pushed the CPLAT-12194-functional-consumed-props branch from 9a65f89 to 7c252f5 Compare October 9, 2020 17:30

joebingham-wk commented Oct 9, 2020

View reviewed changes

example/builder/src/new_class_consumed_props.dart Outdated Show resolved Hide resolved

Remove accidental change

c9228d2

greglittlefield-wf reviewed Oct 9, 2020

View reviewed changes

joebingham-wk added 4 commits October 13, 2020 16:05

Address API feedback

9c7ebbe

Update builder

ea0e4aa

Add generated files

8367f37

Update codegen fixtures

25d6639

greglittlefield-wf reviewed Oct 14, 2020

View reviewed changes

joebingham-wk added 3 commits October 14, 2020 16:33

Add basic props meta tests

cf2cdd3

Add tests for forwardUnconsumedPropsV2

1aaa6c2

Add builder helper tests

7f801fa

joebingham-wk force-pushed the CPLAT-12194-functional-consumed-props branch from 728f513 to 7f801fa Compare October 15, 2020 01:42

joebingham-wk added 2 commits October 15, 2020 08:54

Add functional consumed props tests

ec43447

Deprecate UiPropsMapView

88e20b7

joebingham-wk commented Oct 15, 2020

View reviewed changes

joebingham-wk added 3 commits October 15, 2020 10:12

Fix misc. oversights

c640c03

Add ignore comment

d57eea7

Rename 'meta' to 'staticMeta'

d1b2c9b

joebingham-wk marked this pull request as ready for review October 15, 2020 20:57

greglittlefield-wf requested changes Oct 19, 2020

View reviewed changes

joebingham-wk added 5 commits October 20, 2020 07:43

Remove inList()

501b2c8

Remove UiPropsMeta extension

d28a61e

Update docs

3377f27

Address misc feedback

70086af

Fix a couple small docs typos

91f9be0

greglittlefield-wf requested changes Oct 22, 2020

View reviewed changes

lib/src/component_declaration/builder_helpers.dart Outdated Show resolved Hide resolved

lib/src/util/react_util.dart Outdated Show resolved Hide resolved

Move APIs

b50b8c4

greglittlefield-wf requested changes Oct 22, 2020

View reviewed changes

lib/src/component/strictmode_component.dart Outdated Show resolved Hide resolved

lib/src/util/react_util.dart Show resolved Hide resolved

Remove unnecessary overrides

cf36f6b

greglittlefield-wf approved these changes Oct 23, 2020

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Oct 23, 2020

aaronlademann-wf approved these changes Oct 24, 2020

View reviewed changes

rmconsole-wf added the RM Ready label Oct 26, 2020

rmconsole-wf approved these changes Oct 26, 2020

View reviewed changes

rmconsole7-wk merged commit d094d02 into master Oct 26, 2020

rmconsole7-wk deleted the CPLAT-12194-functional-consumed-props branch October 26, 2020 05:53

rmconsole7-wk mentioned this pull request Oct 26, 2020

RM-85606 RM-85605 Release over_react 3.10.2 #636

Closed

rmconsole-readonly-wk mentioned this pull request Oct 28, 2020

RM-85606 RM-85605 Release over_react 3.11.0 #640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPLAT-12194 Enable functional components to use consumed props #633

CPLAT-12194 Enable functional components to use consumed props #633

joebingham-wk commented Oct 9, 2020 •

edited

Loading

aviary-wf commented Oct 9, 2020

greglittlefield-wf Oct 9, 2020

joebingham-wk Oct 14, 2020

greglittlefield-wf Oct 9, 2020

joebingham-wk Oct 9, 2020

joebingham-wk Oct 14, 2020

greglittlefield-wf left a comment

greglittlefield-wf Oct 14, 2020

greglittlefield-wf Oct 14, 2020

joebingham-wk Oct 15, 2020

joebingham-wk Oct 15, 2020

joebingham-wk Oct 15, 2020 •

edited

Loading

joebingham-wk Oct 15, 2020

greglittlefield-wf left a comment

greglittlefield-wf left a comment

greglittlefield-wf commented Oct 23, 2020

aaronlademann-wf left a comment

joebingham-wk commented Oct 26, 2020

rmconsole-wf left a comment

CPLAT-12194 Enable functional components to use consumed props #633

CPLAT-12194 Enable functional components to use consumed props #633

Conversation

joebingham-wk commented Oct 9, 2020 • edited Loading

Motivation

Changes

STILL LEFT TO DO:

Release Notes

Review

QA Checklist

Merge Checklist

aviary-wf commented Oct 9, 2020

Security Insights

Action Items

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

joebingham-wk Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Oct 23, 2020

aaronlademann-wf left a comment

Choose a reason for hiding this comment

joebingham-wk commented Oct 26, 2020

rmconsole-wf left a comment

Choose a reason for hiding this comment

joebingham-wk commented Oct 9, 2020 •

edited

Loading

joebingham-wk Oct 15, 2020 •

edited

Loading