Getting Workiva/OverReact on Build Runner

[matanlurey]

We spoke to @greglittlefield-wf at DartConf, and we wanted to start this working issue.

Things to cover:

1. Examples of how OverReact uses transformers today.
2. Examples of how OverReact relies on same-file transformations
3. Potential workarounds to use package:build and build_runner, instead.

/cc @natebosch @jakemac53.

[jakemac53]

Some basic docs from @greglittlefield-wf https://github.com/Workiva/over_react/blob/master/lib/src/transformer/README.md#overreact-transformer

[matanlurey]

Started some working documents here:
https://github.com/dart-lang/build/blob/master/docs/transforming_code.md

[matanlurey]

Hey @greglittlefield-wf, have you had any time to consider options here?

[greglittlefield-wf]

Hey @matanlurey, I've been thinking about it in the back of my mind, but haven't had a chance to dig in to the code yet, sorry! I'll try to do that this week and get back to you

[greglittlefield-wf]

Hey @matanlurey, sorry for the delayed response!

I've taken a look, and a large portion of OverReact's code generation (basically all of the steps listed here) plays nicely with or can easily be adapted to work with the restrictions of the build package.

However, there's a case of inline code transformation that doesn't seem to have a super clear path forward using build.

I've split the concerns around this transformation into two parts:

1. Leaving behind fields
2. Inheritance

If you have any ideas for these, or thoughts on my thoughts, I'm all ears!

Also, I realize that some of the restrictions in build will require rethinking how things work fundamentally, and I'm totally open to that! However, migrating in a way that doesn't shake up too much all of the existing code and patterns that rely on what our transformer can do would be nice 😄.

Apologies for the strikethroughs and stream-of-connsciousness-eque format of the following sections; I thought it would be a good idea to ensure I've shared all of my thoughts/ideas, even the bad ones!

Leaving behind fields

We are currently using field syntax to declare props, which the transformer replaces with getters/setters that proxy Map key-value pairs.

 // Current, with transformer
 @Props()
 class FooProps extends UiProps { 
-  String foo;
+  String get foo        => props['foo'];
+  set foo(String value) => props['foo'] = value;
}

Something similar is possible using build, but it leaves behind those fields in the superclass.

 // Using build package
 @Props()
 class FooProps extends UiProps {
   String foo;
 }
+// .g.dart
+class $FooPropsImpl extends FooProps {
+  @override String get foo        => props['foo'];
+  @override set foo(String value) => props['foo'] = value;
+}

The same thing could be accomplished by stubbing props with abstract getters/setters instead of fields, but that would increase the boilerplate of declaring props significantly, which would be quite unfortunate. Syntactic sugar for "abstract fields" could potentially help here, but that's a long shot.

It's also nice to have them be concrete, since that allows the @Props classes to remain non-abstract, and we'll get analyzer warnings if true abstract props declarations aren't implemented. I suppose warnings would show themselves in generated code, though, now that I think about it...

We already have runtime checks in place that preventing instantiation of non-generated UiProps subclasses, though, so users instantiating those classes and accessing those fields shouldn't be a big problem (I think).

There's still the concern of users calling super.{prop} and getting at the field, which would not be good.

It also looks like while these fields get tree-shaken in dartjs, they still show up in JS constructor initializer lists, which isn't ideal (see dart-lang/sdk#11558).

Inheritance

Separate from how those props are declared, however, are concerns around how things would work with inheritance.

With the transformer, everything gets transformed in-place. Thus, extending from and mixing in classes that declare props works pretty well:

@Props()
class FooProps extends UiProps { 
  String foo;
}
@PropsMixin()
abstract class BarPropsMixin {
  int bar;
}
@Props()
class BazProps extends FooProps with BarPropsMixin {
  List baz;
}

With build restrictions, though, this gets a bit more complicated. Here are some of the thoughts I've had, in chronological order:

1. You would have to inherit from the the generated class to get those accessor overrides.

   @Props()
   class FooProps extends UiProps { 
     String foo;
   }
   @PropsMixin()
   abstract class BarPropsMixin {
     int bar;
   }
   @Props()
   class BazProps extends $FooPropsImpl with $BarPropsMixinImpl {
     List baz;
   }
   // .g.dart
   class $FooPropsImpl extends FooProps { /* accessors, etc. */ }
   abstract class $BarPropsMixinImpl implements BarPropsMixin { /* accessors, etc. */ }
   class BazProps extends BazProps { /* accessors, etc. */ }

   This would be confusing for users, and would be difficult to ensure that users don't use the base classes without their generated counterparts.

2. You could potentially use generated mixins instead to perform the accessor overrides, and require users to mix them in.

   @Props()
   class FooProps extends UiProps with $FooPropsAccessors { 
     String foo;
   }
   @PropsMixin()
   abstract class BarPropsMixin extends Object with $BarPropsAccessors {
     int bar;
   }
   @Props()
   class BazProps extends FooProps with $BazPropsAccessors, BarPropsMixin {
     List baz;
   }
   // .g.dart
   abstract class $FooPropsAccessors { /* accessors */ }
   abstract class $BarPropsAccessors { /* accessors */ }
   abstract class $BazPropsAccessors { /* accessors */ }

   This added boilerplate isn't great, but validation might be easier.
   We'd also have to do something different for prop mixins than shown above, since mixins are still restricted in dart2js.
   I just realized this wouldn't work for concrete prop declarations (as is the case with fields), since the fields would trump the accessors declared in the mixins.

3. You could do something similar to Write a transformer wrapper to run in pub serve/build #2, but traverse the inheritance tree to find all prop superclasses and mixins, and then have the generator mix in corresponding generated classes to get the generated accessor implementations.

   @Props()
   class FooProps extends UiProps { 
     String foo;
   }
   @PropsMixin()
   abstract class BarPropsMixin {
     int bar;
   }
   @Props()
   class BazProps extends FooProps with BarPropsMixin {
     List baz;
   }
   // .g.dart
   abstract class $FooPropsAccessors { /* accessors */ }
   abstract class $BarPropsAccessors { /* accessors */ }
   abstract class $BazPropsAccessors { /* accessors */ }
   class $FooPropsImpl extends FooProps with $FooPropsAccessors { /* etc. */ }
   class $BazPropsImpl extends BazProps 
       with $FooPropsAccessors, $BarPropsMixinAccessors, $BazPropsAccessors { /* etc. */ }

   I'm still trying to think if mixing in everything like that at the end would still result in everything falling into place correctly... Seems like it might be fine.

   Also, is this pattern of mixin usage detrimental to dart2js output speed/size or runtime memory usage? We can investigate this.

   This would also prevent us from mixing these accessors into non-generated classes, which we currently do occasionally.

3 is my favorite option so far... I was getting a bit discouraged until I thought of it :D.

[matanlurey]

Hey @greglittlefield-wf thanks for the writeup!

One idea, if you used redirecting factory constructors it could work:

@Props()
abstract class FooProps extends UiProps { 
  factory PooProps() = _$FooProps;

  String foo;
}

// .g.dart
class _$FooProps implements FooProps {
  // ...
}

/cc @natebosch

[jakemac53]

Another thing that you can do, which is a bit of an anti-pattern according to some but might help the mixin case, is actually generate the public FooProps class, and make implementors write the _$FooProps abstract class:

@Props()
abstract class _$FooProps implements UiProps { 
  String foo;
}

// .g.dart
class FooProps extends UiProps implements _$FooProps {
  // ...
}

That requires less boilerplate for the user (no mention of generated classes, no redirecting factory constructor), but they do have to follow a specific naming convention. For consumers I believe it means they can just mixin/extend the FooProps class, but if they navigate to it they will be in a generated file. I know some people don't like that aspect of it, but I don't personally see it as being a huge issue.

[greglittlefield-wf]

@matanlurey Yeah, those are definitely on the table!

With out current setup, though, we probably wouldn't need to add redirecting factory constructors, since we only allow instantiation of those classes under the hood.

UiFactory<FooProps> Foo = $Foo;
@Props()
abstract class FooProps extends UiProps { 
  String foo;
}

// .g.dart
FooProps $Foo() => new $FooPropsImpl();

// usage
render() {
  FooProps builder = Foo();
  new FooProps(); // this was never allowed, and threw a runtime exception
}

[greglittlefield-wf]

@jakemac53 Ooh, interesting, I hadn't thought about doing it "backwards" like that... I'll definitely explore that idea, thanks!

That implements may result in this dart2js issue dart-lang/sdk#14729, but maybe not. Either way, hopefully it gets fixed at some point 😄.

[matanlurey]

Ah I like that too.

re: dart-lang/sdk#14729... no idea.

[greglittlefield-wf]

Update: we haven't spent much time recently on this problem, but we are starting to think about this more, and are hoping to get to it in Q3.

Thanks again for all of the help!

[corwinsheahan-wf]

@matanlurey OverReact 2.0.0 has been released which converts the previous transformer functionality to a builder and is Dart 2 compatible. This issue can now be closed. Many thanks from all of us at Workiva for the help!

[natebosch]

Happy to hear its working!