CPLAT-4031: Make builder compatible with Dart 2 only boilerplate #227

corwinsheahan-wf · 2019-02-01T20:55:07Z

Ultimate problem:

Currently the 2.0.0-dev branch only contains support for building on the transition boilerplate.

How it was fixed:

Add support for Dart 2 only boilerplate by generating consumable companion class for @Props(), @State(), @AbstractProps() and @AbstractState() annotated classes, as well as generated consumable class for props mixins.
Is still backwards compatible with the transition boilerplate.

Testing suggestions:

CI Passes (tests and examples added)

Potential areas of regression:

Everything

FYA: @greglittlefield-wf @aaronlademann-wf @evanweible-wf @maxwellpeterson-wf @sebastianmalysa-wf @georgelesica-wf

TODO: Add tests for decl parsing, impl_generation, and builder that test dart 2 only functionality. (already have integration tests for dart 2 only)

…er_react into dart2_only

…nly bp

aviary-wf · 2019-02-01T20:55:25Z

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

Looked at everything but the tests, found a couple small things but otherwise looks great!

lib/src/builder/generation/declaration_parsing.dart

lib/src/builder/generation/impl_generation.dart

lib/src/builder/generation/declaration_parsing.dart

lib/src/builder/generation/impl_generation.dart

lib/src/builder/generation/declaration_parsing.dart

lib/src/builder/generation/impl_generation.dart

...eact/component_declaration/builder_integration_tests/abstract_accessor_integration_test.dart

.travis.yml

lib/src/builder/generation/declaration_parsing.dart

evanweible-wf · 2019-02-06T01:52:41Z

lib/src/builder/generation/declaration_parsing.dart

+            updateCompanionClass(name, true);
+            validateMetaField(companionClass, isPropsClass(name) ? 'PropsMeta': 'StateMeta');
+          } else {
+            if (!member.name.name.startsWith(privateSourcePrefix)) {


On Dart 2, this validation should happen regardless of whether or not the companion class exists. The class should always be prefixed with _$

In other words, I think this check should be moved up to line ~145

evanweible-wf · 2019-02-06T02:06:23Z

lib/src/builder/generation/declaration_parsing.dart


-      if (variable.initializer != null &&
-          !expectedInitializer.contains(variable.initializer.toString())) {
+      if (variable.initializer != null && expectedInitializer != variable.initializer.toString()) {


Just occurred to me that this misses empty initializers, which we shouldn't support anymore

Suggested change

if (variable.initializer != null && expectedInitializer != variable.initializer.toString()) {

if (variable.initializer == null || variable.initializer.toString() != expectedInitializer) {

lib/src/builder/generation/impl_generation.dart

test/vm_tests/builder/impl_generation_test.dart

aaronlademann-wf

This is... impressive work @corwinsheahan-wf!

So close... so very, very close!!!

evanweible-wf · 2019-02-07T00:38:19Z

lib/src/builder/generation/declaration_parsing.dart

+        if ((variable?.initializer?.toString() ?? '') != expectedInitializer) {
+          error(
+              'Factory variables are stubs for the generated factories, and should not have initializers '
+                  'unless initialized with a valid variable name for Dart 2 builder compatibility. '


This message is leaking some legacy. I think we can drop the "...should not have initializers unless initialized with..." part and just tell them that they "should be initialized with ".

evanweible-wf · 2019-02-07T00:59:05Z

@corwinsheahan-wf Just noticed this when reading the docs updates - any reason the generated factory implementation needs to be public?

over_react/lib/src/builder/generation/impl_generation.dart

Line 526 in afbee2f

return '$publicGeneratedPrefix${componentClassName}Factory';

corwinsheahan-wf · 2019-02-07T01:12:58Z

@evanweible-wf That does need to be public. When a component is a subtype of another component, the super component's factory is referenced as part of the factory for the sub component:

parentType: $SuperComponentFactory,

The super and sub components can, and will often be, in separate libs

aaronlademann-wf

+1

Comments have been addressed

corwinsheahan-wf

+1 to Evan's commit

evanweible-wf · 2019-02-07T20:05:44Z

QA +1

CI passes
Verified locally against two internal consumers using backwards-compat boilerplate and the dart2-only boilerplate.

@Workiva/release-management-p

corwinsheahan-wf added 7 commits January 29, 2019 18:36

Generate for dart 2 code, tests still wip

aacaf6a

Merge branch 'builder_generate_public_class' into dart2_only

4cd2755

Fix broken tests

3a4667e

TODO: Add tests for decl parsing, impl_generation, and builder that test dart 2 only functionality. (already have integration tests for dart 2 only)

Merge branch 'builder_generate_public_class' into dart2_only

475e4c5

Merge branch 'builder_generate_public_class' of github.com:Workiva/ov…

6100d1e

…er_react into dart2_only

Update decl parsing test; Create source files for builder on Dart 2 o…

47cc9c4

…nly bp

Update rest of tests; make examples Dart 2 only

00b0457

corwinsheahan-wf requested a review from a team as a code owner February 1, 2019 20:55

corwinsheahan-wf changed the base branch from master to 2.0.0-dev February 1, 2019 20:55

rmconsole-wf added the Merge Requirements Unmet label Feb 1, 2019

greglittlefield-wf previously requested changes Feb 1, 2019

View reviewed changes

evanweible-wf requested changes Feb 3, 2019

View reviewed changes

corwinsheahan-wf added 5 commits February 5, 2019 15:24

Make builder copy over static fields/methods

59c9e22

Copy over class members in mixins

bb1d862

convert to Dart 2 only source code

ac35d71

Add tuneup for analysis check to travis.yml

d6208f0

copy over non-generated fields; delete unused integrate folder

c8e3fe9

evanweible-wf reviewed Feb 6, 2019

View reviewed changes

aaronlademann-wf reviewed Feb 6, 2019

View reviewed changes

address review comments

efa1b49

evanweible-wf reviewed Feb 7, 2019

View reviewed changes

Update error message to be Dart 2 specific

afbee2f

fix tests + review feedback

6814952

evanweible-wf approved these changes Feb 7, 2019

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Feb 7, 2019

Tweak generate_for sources for performance.

ed60b5e

rmconsole-wf added Merge Requirements Unmet and removed Merge Requirements Met labels Feb 7, 2019

aaronlademann-wf approved these changes Feb 7, 2019

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Feb 7, 2019

corwinsheahan-wf commented Feb 7, 2019

View reviewed changes

rmconsole-wf added the RM Ready label Feb 7, 2019

rmconsole2-wf merged commit 1b00666 into 2.0.0-dev Feb 7, 2019

rmconsole2-wf deleted the dart2_only branch February 7, 2019 20:06

rmconsole-readonly-wk mentioned this pull request Feb 8, 2019

CPLAT-246 CPLAT-245 Release over_react 2.0.0-alpha #239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPLAT-4031: Make builder compatible with Dart 2 only boilerplate #227

CPLAT-4031: Make builder compatible with Dart 2 only boilerplate #227

corwinsheahan-wf commented Feb 1, 2019 •

edited

Loading

aviary-wf commented Feb 1, 2019

greglittlefield-wf left a comment

evanweible-wf Feb 6, 2019

evanweible-wf Feb 6, 2019

evanweible-wf Feb 6, 2019

aaronlademann-wf left a comment

evanweible-wf Feb 7, 2019

evanweible-wf commented Feb 7, 2019

corwinsheahan-wf commented Feb 7, 2019 •

edited

Loading

aaronlademann-wf left a comment

corwinsheahan-wf left a comment

evanweible-wf commented Feb 7, 2019

	if (variable.initializer != null && expectedInitializer != variable.initializer.toString()) {
	if (variable.initializer == null \|\| variable.initializer.toString() != expectedInitializer) {

CPLAT-4031: Make builder compatible with Dart 2 only boilerplate #227

CPLAT-4031: Make builder compatible with Dart 2 only boilerplate #227

Conversation

corwinsheahan-wf commented Feb 1, 2019 • edited Loading

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

aviary-wf commented Feb 1, 2019

Security Insights

Action Items

greglittlefield-wf left a comment

Choose a reason for hiding this comment

evanweible-wf Feb 6, 2019

Choose a reason for hiding this comment

evanweible-wf Feb 6, 2019

Choose a reason for hiding this comment

evanweible-wf Feb 6, 2019

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

evanweible-wf Feb 7, 2019

Choose a reason for hiding this comment

evanweible-wf commented Feb 7, 2019

corwinsheahan-wf commented Feb 7, 2019 • edited Loading

aaronlademann-wf left a comment

Choose a reason for hiding this comment

corwinsheahan-wf left a comment

Choose a reason for hiding this comment

evanweible-wf commented Feb 7, 2019

corwinsheahan-wf commented Feb 1, 2019 •

edited

Loading

corwinsheahan-wf commented Feb 7, 2019 •

edited

Loading