Skip to content

CPLAT-11530: Migrate to new over_react boilerplate#103

Merged
rmconsole7-wk merged 8 commits into
masterfrom
over_react_boilerplate_migration
Jul 16, 2020
Merged

CPLAT-11530: Migrate to new over_react boilerplate#103
rmconsole7-wk merged 8 commits into
masterfrom
over_react_boilerplate_migration

Conversation

@sydneyjodon-wk
Copy link
Copy Markdown
Contributor

These changes / builds will first be reviewed by a member of the Client Platform team.

Repo owners will be notified when it is ready for additional review, merge and release.

Motivation

While converting the boilerplate for OverReact component declaration from Dart 1 transformers to Dart 2 builders, we encountered several constraints that made us choose between dropping backwards compatibility (mainly support for props class inheritance), and a less-than-optimal boilerplate.

To make the Dart 2 transition as smooth as possible, we chose to keep the new boilerplate version as backwards-compatible as possible, while compromising the cleanliness of the boilerplate. In time, we found that this wasn't great from a user experience or from a tooling perspective.

Knowing this, and having dropped support for Dart 1, we have implemented an improved version of OverReact boilerplate that fixes issues introduced in the "transitional" version, as well as other miscellaneous ones.

For more details, check out the New OverReact Boilerplate Guide.

To summarize the new boilerplate changes:

  • Props/state classes are now always declared as mixins, and can't extend from other props classes anymore (but you can still mix them in)!)
  • No more stubbed-in concrete props classes at the end of the file
  • Annotations (Factory(), @Props(), etc.) are omitted unless they have arguments
  • consumedProps includes props from all used props mixins by default
  • uri_has_not_been_generated errors are ignored by default (via workiva_analysis_options)
  • "Go to reference" in IDEs for props now takes you to the source and not the generated code
  • Multiple components per file are allowed (the old boilerplate also gets this, as a bonus!)

Basically...

-// ignore: uri_has_not_been_generated
 part 'foo.over_react.g.dart';

-@Factory()
 UiFactory<FooProps> Foo =
-    // ignore: undefined_identifier
-    _$Foo;
+    _$Foo; // ignore: undefined_identifier

-@Props()
-class _$FooProps extends UiProps {
+mixin FooProps on UiProps {
   String foo;
 }

-@Component2()
 class FooComponent extends UiComponent2<FooProps> {
   @override
   render() => 'foo: ${props.foo}';
 }
-
-// ignore: mixin_of_non_class, undefined_class
-class FooProps extends _$FooProps with _$FooPropsAccessorsMixin {
-  // ignore: undefined_identifier, const_initialized_with_non_constant_value
-  static const PropsMeta meta = _$metaForFooProps;
-}

Changes

(These might be a bit easier to read with split diff)

  1. Bump the lower bound of the over_react dependency to pull in the
    builder changes necessary to support the boilerplate changes.
  2. Migrate all compatible component declarations to the new boilerplate.
    • Components that have not yet been migrated to UiComponent2 cannot use the new boilerplate.
    • Components that have publicly exported props or state classes cannot be migrated without causing breaking changes.
  3. Add TODO comments to any part of a declaration that could not be migrated automatically.

Review

Please review, QA and merge: <tag repo owners / contributors here>

QA Checklist

  • Passing CI
  • Verify/smokecheck that Components upgraded to the new boilerplate are still functioning as expected.

Release Notes

  • Migrate compatible component declarations to the new OverReact boilerplate.

More information about the new OverReact component boilerplate

@aviary3-wk
Copy link
Copy Markdown

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.


@Props()
class _$WrapperProps extends UiProps {}
mixin WrapperProps on UiProps {}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated WrapperProps without deprecating it because nothing ever extends from it.

Copy link
Copy Markdown
Contributor Author

@sydneyjodon-wk sydneyjodon-wk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I upgraded some of the test components to UiComponent2 and the new boilerplate, but it seemed like some should stay in the old boilerplate. Based on the test descriptions, the following components seemed like they were being used to test legacy boilerplate or UiComponent so I didn’t upgrade these:
Test, TestCommon, TestCommonNested, TestCommonNested2, TestCommonRequired

Let me know if I should’ve upgraded any of these or if any of the components I upgraded shouldn’t have been.

@sydneyjodon-wk sydneyjodon-wk marked this pull request as ready for review July 2, 2020 23:38
Copy link
Copy Markdown
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+10

@Workiva/release-management-p

Copy link
Copy Markdown

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from RM

@rmconsole7-wk rmconsole7-wk merged commit 21ca3a9 into master Jul 16, 2020
@rmconsole7-wk rmconsole7-wk deleted the over_react_boilerplate_migration branch July 16, 2020 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants