Skip to content

CPLAT-1858, CPLAT-1857: Builder to build on backwards compatible boilerplate #211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 147 commits into from
Feb 1, 2019

Conversation

corwinsheahan-wf
Copy link
Contributor

@corwinsheahan-wf corwinsheahan-wf commented Dec 18, 2018

Ultimate problem:

Dart 2 drops support for transformers

How it was fixed:

Add an analogous builder (factory name overReactBuilder) which provides the same functionality and operates on boilerplate which is compatible with both Dart 1 and Dart 2. See the section about transition boilerplate in the Migration Guide

Testing suggestions:

  • Tests added which cover build output
  • Functionality tested on internal repositories using over_react.

Potential areas of regression:

  • Pretty much everything

FYA: @greglittlefield-wf @aaronlademann-wf @kealjones-wk @evanweible-wf @maxwellpeterson-wf

greglittlefield-wf and others added 30 commits February 13, 2018 20:53
Copy link
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.

Done with first pass, looking good

' @override\n'
// Don't type this so that it doesn't interfere with classes with generic parameter props type:
// class FooComponent<T extends FooProps> extends UiComponent<T> {...}
// TODO use long-term solution of component impl class instantiated via factory constructor
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf Jan 30, 2019

Choose a reason for hiding this comment

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

This is actually a TODO leftover from the transformer; the problem occurred when extending a non-abstract component, which had a generated implementation of typedPropsFactory mixed in.

class FooComponent<T extends FooProps> extends UiComponent<T>
  // mixin added by transformer
  with _$FooComponentMixin { ... }
class _$FooComponentMixin {
   FooProps typedPropsFactory(map) => new _$FooPropsImpl(map);
}

class BarProps extends FooProps {}
class BarComponent extends FooComponent<BarProps>
  // mixin added by transformer
  with _$BarComponentMixin { ... }
class _$BarComponentMixin {
   BarProps typedPropsFactory(map) => new _$BarPropsImpl(map);
}

Then it would complain something like

typedPropsFactory in class BarProps (Map) => BarProps is not a valid override of
typedPropsFactory in class FooProps (Map) => <T extends FooProps>

Since BarProps isn't always a valid subtype of T.

Edit, sorry, I got it kind of backwards. The error would be:

typedPropsFactory in class FooProps (Map) => FooProps is not a valid override of
typedPropsFactory in class UiProps (Map) => <T extends FooProps>

Since FooProps isn't always a valid subtype of T.


Now with the way that the builder generates the typedPropsFactory in an additional subclass that is never subclassed fromprovides a type for T, we shouldn't hit that conflict in BarPropsFooProps!

Theoretically, you should be able to reinstate the typing (or, since you don't need to redeclare the typing on overrides, just remove the as dynamic) and it'll work!

-typedPropsFactory(Map backingMap) => new $propsImplName(backingMap) as dynamic;
+typedPropsFactory(Map backingMap) => new $propsImplName(backingMap);

@@ -6,59 +6,37 @@ authors:
- Workiva UI Platform Chapter <[email protected]>
- Workiva App Frameworks Team <[email protected]>
environment:
sdk: '>=2.0.0-dev.0 <=2.0.0-dev.49.0'
sdk: '>=2.1.0 <3.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought these changes were slated for Dart 1 and Dart 2 SDK compat? Shouldn't the lower bound remain >=1.24.3 in that case? Or am I misunderstanding the milesone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Transformers can't exist in a Dart 2 repo b/c barback doesn't have any Dart 2 compatible versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

mmm... we should probably change the milestone description then, IMO. It says pretty clearly that we're targeting dart 1 and dart 2 support, which sounds like it isn't possible.

Copy link
Contributor

@evanweible-wf evanweible-wf Jan 31, 2019

Choose a reason for hiding this comment

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

We will still effectively be supporting both Dart 1 and Dart 2, but the method for doing so will be the dual release lines (eg. 2.0.0-dart1 and 2.0.0-dart2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, except it's +, not -, right? Also I believe that the plan is to have 2.0.0 and 2.0.0+dart1 (i.e. Dart 2 is the default)

Copy link
Contributor

Choose a reason for hiding this comment

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

How will the dual release lines work if the SDK lower bound (and other dependency ranges) are Dart 2 only @evanweible-wf ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's actually how the dual release lines work. For every version that we want to release, we'll actually cut two releases. The 2.0.0-dev branch will be our divergence point – we'll branch off of current master to make a master_dart1 branch before we merge 2.0.0-dev in and we'll cut the dart1 releases off of that branch and dart2 releases off of master. So, for example, the 2.0.0 release would look like this:

  • 2.0.0-dart1
    • tagged from dart1_master branch
    • environment constraint of >=1.24.3 <2.0.0
    • uses the transformer
  • 2.0.0-dart2
    • tagged from master branch
    • environment constraint of >=2.1.0 <3.0.0
    • uses the builder

As a consumer, this is all transparent once deprecation warnings have been taken care of. We will depend on a version range like we normally do:

dependencies:
  over_react: ^2.0.0

And then pub will pull in the appropriate version based on the version of the Dart SDK that is installed. This is how the dart2_constant package works.

typeParameters.typeParameters.map((t) => t.name.name).join(
', '))..write('${typeParameters.rightBracket}'))
.toString()
: '';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of those blocks of code that ... when you see it... you wonder - WITAF is dart format doing?

Like... how could this possibly be the "canonical" way to format this code?

@corwinsheahan-wf corwinsheahan-wf changed the title AF-1858, AF-1857: Builder generate public class CPLAT-1858, CPLAT-1857: Builder to build on backwards compatible boilerplate Feb 1, 2019
@evanweible-wf
Copy link
Contributor

QA +1

  • CI passes

Will thoroughly test this on our internal consumers before merging 2.0.0-dev to master.

@Workiva/release-management-p

Copy link
Contributor Author

@corwinsheahan-wf corwinsheahan-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 to others' commits

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.

8 participants