Code generator design

[katis]

Since we've started working on the code generation, I thought I'd open a discussion thread about its design.

We need at least three working annotations, @observable, @computed and @action.

The generator will find all classes with a specific annotation (@observable, @store?) and creates a private subclass _$ParentClass that overrides the annotated fields, getters and methods with Mobx magic.

I'll use parts of this annotated class in the next sections:

@store // tbd
class User {
  @observable
  String firstName = 'Jane';

  @observable
  String lastName = 'Doe';

  @computed
  String get fullName => '$firstName $lastName';

  @action
  void updateNames({String firstName, String lastName }) {
    ...
  }
}

@observable

The first approach I tried was with a hidden Observable<T> field:

class _$User {
  final _$firstName = Observable<String>(null, name: 'User.firstName');

  @override
  String get firstName => _$firstName.value;

  @override
  set firstName(String value) => _$firstName.value = value;
}

This had some downsides. One was that the User creates storage for the original firstName field which then ends up unused. Second is that the initialization becomes slightly more complicated. The initial value must be copied from the superclasses field, and the field must be nulled so that it doesn't keep the initial value in memory. Code would look something like this:

class _$User {
 _$User() {
   _$firstName.value = super.firstName;
   super.firstName = null;
 }

 final _$firstName = Observable<String>(null, name: 'User.firstName');

  @override
  String get firstName => _$firstName.value;

  @override
  set firstName(String value) => _$firstName.value = value;
}

My second attempt was with an Atom which made the initialization a non-issue, since it just uses the original field:

class _$User {
 final _$firstNameAtom = Atom(name: 'User.firstName');

  @override
  String get firstName {
     _$firstNameAtom.reportObserved();
    return super.firstName;
  }

  @override
  set firstName(String value) {
    super.firstName = value;
    _$firstNameAtom.reportChanged();
  }
}

I think the Atom method is better, for the reasons above.

@computed

Computed should be simple, just override a getter and use the original one:

class _$User {
  _$User() {
    _$fullNameComputed = Computed(() => super.fullName);
  }

 Computed<String> _$fullNameComputed;

  @override
  String get fullName => _$fullNameComputed.value;
}

@action

The @action annotation has a name conflict with the current action() builder. We could remove action() and replace it with multiple action variants for different function arities: action0<R>(() => ...), action1<A, R>((a: A) => ...), action2<A, B, R>((a: A, b: B) => ...). The names are ugly, but they could be used by the codegen and I think that once the code generator is done, people won't use the raw versions much.

class _$User {
  _$User() {
    _$updateNames = action2((String a, String b) => super.updateNames(firstName: a, lastName: b));
  }

  Action2<String, String ,void> _$updateNames;

  @override
  void updateNames({String firstName, String lastName}) {
    _$updateNames(firstName, lastName);
  }
}

Named vs. positional arguments will add complexity in the generator.

Automatic conversions

Mobx JS makes objects and arrays automatically observable when you use the @observable decorator.
You can opt-out by using @observable.ref decorator instead.

Dart can't support the feature for classes, but it would be possible (sort of) for collections like Maps and Lists.

I'm not a fan of it as a default, and using an ObservableList is not much harder than using a List.

class ShoppingCart {
  @observable
  List<Item> items = [];
}

vs.

class ShoppingCart {
  @observable
  List<Item> items = ObservableList();
}

Since we can't modify the actual list assigned to the variable like in JS but only wrap it, it might be confusing that the non-observable List you passed to ShoppingCart.items is suddenly a reactive list when accessed from items.

final items = [];

final cart = ShoppingCart();
cart.items = items; // Supposedly items and cart.items are the same list

cart.items.add(Item()); // Mobx reacts to the change :)

items.add(Item()); // Mobx misses the change :(

Constructors

The generator should be able to create a constructor that matches the superclass factory that delegates to the subclass:

class User {
  // 
  User._(this.firstName, this.lastName);

  // Dart can delegate to the subclass constructor with this syntax
  factory User({String firstName, String lastName}) => _$User; // This could also determine the name of the subclass?
}

class _$User {
  _$User({String firstName, String lastName})
    : super._(firstName, lastName}) {
  }
}

observe() support in annotated classes

We would want the generated classes to be supported in devtools etc, but the problem is that the Atoms, Computeds etc are hidden in the subclass. It would be ugly to expose them as public, and you'd have to cast the User to _$User.

Another way would be to define an interface like:

abstract class Store {
  void observe(String field, listener);
}

make the annotated class abstract, and implement Store:

abstract class User implements Store {
}

Then the code generation could implement Store automatically:

class _$User extends User {
  void observe(String field, listener) {
    if (field == 'firstName') { ... }
    // etc.
  }
}

Store interface could also expose a Map of observable fields, so devtools could iterate and analyze them.

[katis]

I've requested that the #46 branch would be merged, it has the package structure and configs already, so should work as a starting point.

[katis]

I experimented with generating actions, and I think that the easiest way to support generics, optional parameters and named parameters is to skip the Action wrapper as it currently exists, and use the start-finally-stop pattern we use with the observable hook.

class User {
  @action
  void updateNames({String firstName, String lastName}) {
    if (firstName != null) this.firstName = firstName;
    if (lastName != null) this.lastName = firstName;
  }
}

Generates:

class _$User {
  final _$UserActionController = ActionController(name: 'User');

  @override
  void updateNames({String firstName, String lastName}) {
    final _$prevDerivation = _$UserActionController.startAction();
    try {
      return super.updateNames(firstName: firstName, lastName: lastName);
    } finally {
      _$UserActionController.endAction(_$prevDerivation);
    }
  }
}

[copydog]

@katis I agree with your idea, we should merge the branch (@pavanpodila ).

Few my opinion

• This is my way to deal with ObservableList

@observable
List<String> favoriteColorList = [
  'red',
  'blue',
  'yellow',
];

/// Generated
ObservableList<String> _favoriteColorList;

@override
set favoriteColorList(List<String> favoriteColorList) {
    _favoriteColorList
      ..clear()
      ..addAll(favoriteColorList);
}

List<String> get favoriteColorList => _favoriteColorList;

/// constructor code is not posted
/// initialize value and nulled user-defined value will be done in constructor

• I prefer Observable to Atom, because using Atom MIGHT lose the intercept and value change checking ability (I am not very sure, correct me if I am wrong). And all the work that you described is already done in mobx_generator branch, I will start to migrate these code once your pull request is merged.

• I suggest we use mustache4dart library. So we can maintain the template easily (you can find in mobx_generator branch)

Few things that need to discuss:

1. What is the class decorator name, store or observable ? I prefer store, It's more clear for user.
2. How to solve action conflict?

[pavanpodila]

@katis Firstly, thanks for an educative discourse on the options you took. I have personally learnt a few interesting things around the use of factory constructors.

Actions

As regards action, I think we should forgo that name in favor of the Action() constructor (note capital A). We can then reserve the word action for the annotation.

Inject annotation

For the store annotation, I think we should adopt what MobX has done instead: inject. With inject we can supply multiple types of store injections. It could be:

• name based: in which case we look up from a map of stores given to the Provider (we have not implemented yet. But this will be an InheritedWidget that passes down all stores)

@inject('auth')
class SomeWidget extends StatelessWidget { }

• function based: in which case we will pass a collection of stores as input argument, which can then be used to return an object containing the stores we need.

@inject((Map<String, Object> storeMap) => storeMap('auth'))
class SomeWidget extends StatelessWidget { }

Not sure if we can overload the inject annotation. If we can't we should go for a function-based approach.

Some other thoughts

I feel we don't have to adopt all the API conventions from MobX as some may not be valid for Dart. For example, the lack of overloading and dynamic dispatch of a function with variable types of arguments. This is why we are stuck in a world of action0, action1, action2 type of placeholder functions for argument arities.

We should be prudent in picking what makes sense for Dart and stay idiomatic for the language. Curious to hear what you guys think.

[katis]

The @store annotation in the example code is just a marker that the code generator can use to find classes that need Mobx code generation.

[copydog]

@pavanpodila The @store is marked on store class which used by code generator, @inject is used on widget, it's different.

[pavanpodila]

Got it. I think we have some creative license here to improve the the Developer eXperience (DX) of using mobx.dart.

[katis]

@copydog Not familiar with mustache4dart, is there some advantage over using pure Dart string templates?

[copydog]

@katis We can get more clear code for loop and control flow. And allow us to create one template file for each annotation.

here is the file structure that I used, very clean and maintainable

[pavanpodila]

Agree with @copydog. Mustache will also allow us to iterate faster with the generated code. The role of the generator would be to just prepare the context for the mustache template, which is kind of fixed for the most part. How we use that context to generate the final code can keep changing in the template.

[katis]

Sure, mustache is fine by me. My code uses Dart template strings currently, but the code is ugly in a quick&dirty kind of way, so needs refactoring anyways

[rrousselGit]

Inject annotation

Is there really a need for this? Isn't an InheritedWidget enough?

[rrousselGit]

I don't think it's even possible to begin with. The dart way of your function-based example would be this:

Something selector(Map anotherThing) {}

@Inject(selector)
class Foo {}

Closures are not compile time constant.

And we'd need to create a factory for the widget to be able to inject the field; which is not very ideal.

[katis]

Yeah, I doubt it's possible in Dart in the way it works with JS. InheritedWidget or making a little hook that does the same seems enough to me.

[rrousselGit]

Also, would this make sense to offer a simplified @store that consider all variables as @observable, all getters as @computed and methods as @action?

Example:

Instead of:

@store
class Foo {
  factory Foo() = _$Foo;

  @observable
  String foo;
  @computed
  int get length => _foo.length;

  @action
  void someAction() {}
}

we have a simplified:

@store
class Foo {
  factory Foo() = _$Foo;

  String foo;
   int get length => _foo.length;

  void someAction() {}
}

This is much easier to write, which would make users less tempted to make one HUGE store instead of multiple smaller ones.

And it could be used as a replacement for the lack of mirroring to make a useObservable hook for complex objects.

[rrousselGit]

Finally (sorry for the spam), an alternative to the issue about @observable @katis explained is abstract classes:

@store
abstract class Foo {
  Foo._();
  factory Foo() = _$Foo;

  int get computed => observable

  int get observable;
  set observable(int value);

  void action() {}
}

This is now fine for the generated class to use Observable internally:

class _$Foo extends Foo {
  _$Foo(): super._();

  final _$observable = Observable<String>(null, name: 'User.firstName');

  @override
  String get observable => _$observable.value;

  @override
  set observable(String value) => _$observable.value = value;
}

[katis]

I did think about that, but making a separate getter and setter seems too much trouble for a simple field.
I also don't understand why we're trying to force the use of Observable internally since Atom solves pretty much all the problems Observable causes in the first place 😅
If we run into issues with Atom while implementing spying/tracing, we can create a new class for the use case, or do some other refactorings, but for now Atom does everything we want.

[katis]

I think that the idea about the @store is a good, here's my suggestions for it:

Instead of the code generator searching for a class with an annotation, it searches for abstract classes implementing a Store interface. We can add spying/tracing methods to it later, but for now it could just be an empty marker interface.

abstract class User implements Store {
}

If the class implements Store and has an @observable annotation, all the methods/fields get transformed:

@observable
abstract class User implements Store {
}

[pavanpodila]

I like the reuse of @observable at the class level to automatically mark all fields as observable, methods as actions and also consider computed properties. We can also pass arguments into @observable to control the automatic observability!

Actually the arguments to @observable reminds me of the decorate() API of MobX JS.

[rrousselGit]

it searches for abstract classes implementing a Store interface.

What would Store interface define?

---

I think we still have an issue with actions:

class Foo {
  Foo._();
  factory Foo() = _$Foo;  
}

We have to use this trick just so that _$Foo can extend Foo.

This looks pretty bad. And if the user wants to pass custom values to the constructor or define final variables, things get even worse.

[katis]

Currently Store doesn't implement anything, but in the future it might provide an interface for listening the internal values. It would always be automatically implemented by the generator.

Maybe something like:

abstract class Store {
  void observe(String field, listener);
}

The constructor issue is I think unsolvable. built_value has the same issue, its code generation requires this boilerplate:

abstract class Person implements Built<Person, PersonBuilder> {
  Person._();
  factory Person([updates(PersonBuilder b)]) = _$Person;
}

built_value creates a builder so you can initialize it more easily, and that might be the way we have to go if we want ergonomic constructors.

Edit: I think the builder wouldn't help much, built_value needs it because it creates immutable classes.

Darts cascade notation makes leaving the constructor empty bearable:

User()
  ..firstName = 'John'
  ..lastName = 'Johnson';

[rrousselGit]

Currently Store doesn't implement anything, but in the future it might provide an interface for listening the internal values. It would always be automatically implemented by the generator.

Shouldn't this be done by an autorun/reaction instead?
I'm not sure this is a good example, especially in a typed language.

I'm asking because if we never really have a use-case for this, then the interface just adds some noise. A @store is much easier to write

[rrousselGit]

Another possibility is mixins using the shorthand syntax:

class Merged = SomeClass with SomeMixin;

---

We could define a mixin that would be the equivalent of your proposed Store interface:

mixin Store<T> {}

Then the user write:

class Test = _$Test with Store<_Test>;

mixin _Test {
  int foo;
  int get bar => 42;

  void action() {
    print('hello world');
  }
}

And we generate:

abstract class _$Test with _Test {
  int get foo {
    // Subscribe to the value
    return super.foo;
  }
  set foo(int value) {
    // Notify a value changed
    super.foo = value;
  }

  int get bar {
    // Subscribe to the computed value
    return super.bar;
  }
}

---

The Store mixin is quite useless as of now, but it's required since Dart doesn't have a typedef yet (and we can't do class Foo = Bar;).

But at the same time this covers your suggestion of a Store interface for future features.

[katis]

I guess the limitation with a mixin is that the user can't add a constructor at all? I wouldn't be happy with that.

Apparently spying/tracing requires more detailed events about the things happening with observables than you can get with autorun ( https://mobx.js.org/refguide/observe.html ) and the observe method only tracks the single observable, @pavanpodila might have more information. If we want to get access to the internal values, adding accessor methods to Store is probably the cleanest way.

[katis]

I played around with mixins and you could enable user defined constructors like this:

import 'package:mobx/mobx.dart';

class User = UserBase with Store, _$User;

class UserBase {
  final String id;

  UserBase(this.id);

  @observable
  String firstName = 'Jane';
}

mixin _$User on UserBase {
  final _$firstNameAtom = Atom();

  @override
  String get firstName {
    _$firstNameAtom.reportObserved();
    return super.firstName;
  }

  @override
  set firstName(String value) {
    super.firstName = value;
    _$firstNameAtom.reportChanged();
  }
}

But mixins can't have constructors, and final field = Computed(() => super.foo) is not valid field initialization because the lambda accesses super...

[rrousselGit]

Ah interesting, I didn't think of swapping the class/mixin.

But mixins can't have constructors, and final field = Computed(() => super.foo) is not valid field initialization because the lambda accesses super...

Would a lazy initialization works?

T _field;
T get field {
  _field ??= T();
  return _field;
}

[katis]

I suppose it would. I'll take a look at the mixin approach, there might be issues with codegen since super mixins are a new feature that is not fully supported on all platforms (?) dart-lang/language#12

[rrousselGit]

This issue is likely outdated. It says that dartpad doesn't support them, but it actually does.

And only the mixin keyword is new. The class Foo = Bar with Mixin has been there for ages.

If an issue arises (which I doubt), we can generate the old mixin syntax instead:

abstract class _$User extends UserBase {}

[copydog]

@rrousselGit The mixin method is working, I tried it in the mobx_generator branch. The only one downside is the mixin can NOT have a constructor, which means the original class and it's super-class can NOT have any constructor.

[copydog]

I am using mobx_generator branch in my project now. If you guys have time, please take few minutes to check out these examples.

[katis]

I guess you could make them free functions instead? :/

[amrnt]

Fair enough. I'm still trying to understand your comment regarding this

Unfortunately we need super access in the mixin body, so the mixin<->class relationship would be circular, which is forbidden by the compiler.

Why do we really need a this circular relationship? Wouldn't be better if its implemented in another way so we can skip this "forbidden" relationship? I'm just loudthinking here 😄

[katis]

Basically, if the codegen generates a mixin that overrides stuff of the base class the declaration must be

mixin _$User on User {}

Then if the class would use the mixin

class User with _$User {}

This would be circular relationship which won't work.

We could instead reverse the codegen so that the user creates a mixin, and we generate a class for it. This would remove the need for a the mixin application, but then the user couldn't add a constructor. To me that would be a bigger problem because mixin initialization is pretty limited.

I think we should change the codegen a bit that it would search for all uses of the mixin, instead of just the short class Foo = FooBase with _$Foo; syntax. This way you could add your json_serializable stuff to the class:

@JsonSerializable()
class User extends UserBase with _$User {
   User(String id) : super(id); 

   factory User.fromJson(Map<String, dynamic> json) => _$UserFromJson(json);
   Map<String, dynamic> toJson() => _$UserToJson(this);
}

[rrousselGit]

Why do we need to decorate it this way:

@JsonSerializable()
class User = UserBase with _$User;

?

What about this instead:

class User = UserBase with _$User;

@JsonSerializable()
class UserBase {

}

[katis]

Because then the serialization creates a UserBase instead of User (and UserBase has to be abstract, so it can't be instantiated).

[amrnt]

For now it can be done creating a temp class and then inherent it...

@JsonSerializable()
class User extends _TempUser {
  User(String id) : super(id);

  factory User.fromJson(Map<String, dynamic> json) => _$UserFromJson(json);
  Map<String, dynamic> toJson() => _$UserToJson(this);
}

class _TempUser = UserBase with _$_TempUser;

abstract class UserBase implements Store {
  UserBase(this.id);

  final String id;

  @observable
  String firstName = '';

  @observable
  String lastName = '';

  @computed
  String get fullName => '$firstName $lastName';

  @action
  void updateNames({String firstName, String lastName}) {
    if (firstName != null) this.firstName = firstName;
    if (lastName != null) this.lastName = lastName;
  }
}

[rrousselGit]

Because then the serialization creates a UserBase instead of User

Interesting, because the IDE doesn't see any error until we run the code.

It seems doable though:

void main() {
  final f = User.fromJSON();
  
  print(f.foo);
}

class User = UserBase with Foo;

mixin Foo on UserBase {}

class UserBase {
  final String foo;
  UserBase(this.foo);
  
  UserBase.fromJSON(): this('foo');
}

We should probably talk about this use-case on https://github.com/dart-lang/language
There is currently an ongoing debate on extensions, partial classes, and anything similar. These would heavily simplify our issue.

[katis]

@amrnt It was simple to relax the rules of code generation, see #53

[katis]

Maybe I should make a small tweak for the declaration syntax:

class User = UserBase implements Store with _$User;

class UserBase { }

or go back to the annotation

@observable
class User = UserBase with _$User;

class UserBase {}

This way the base class doesn't need to be abstract or contain anything related to Mobx (besides annotations) and it would conceptually be similar to JS:

const user = observable(userBase)

const userBase = {}

We also need a convention for the Base class, for documentation and such. Should it be FooBase, _Foo, PlainFoo or something else?

[pavanpodila]

I guess the limitation with a mixin is that the user can't add a constructor at all? I wouldn't be happy with that.

Apparently spying/tracing requires more detailed events about the things happening with observables than you can get with autorun ( https://mobx.js.org/refguide/observe.html ) and the observe method only tracks the single observable, @pavanpodila might have more information. If we want to get access to the internal values, adding accessor methods to Store is probably the cleanest way.

Conceptually spying and tracing work similar to the onBecomeObserved and onBecomeUnobserved hooks. Observables will report reads and writes to the "spy", which are then relayed back to the outside world. Here the spy is implicitly created and used when someone attaches a handler to a spy() method (not yet implemented)

[pavanpodila]

@katis the approach you suggested is a bit cleaner now. How about the names like below?

@observable
class User = _User with _UserMixin;

class _User {}

[katis]

Tried to implement the discussed syntax, but there's a bug in Dart that prevents it from working: dart-lang/sdk#35011

[pavanpodila]

I was going through a bunch of other packages that do code generation and really like the approach @rrousselGit took for functional_widget.

How functional_widget does it

Essentially he introduces a capitalized Class identifier for a lowercase-named function identifier.

Thus...

@widget
Widget foo(BuildContext context, int value) {
  return Text('$value');
}

generates...

class Foo extends StatelessWidget {
  final int value;

  const Foo(this.value, {Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return foo(context, value);
  }
}

and you use it as...

runApp(
    Foo(42)
);

Proposal for a Store implemented class

What if we take the implementation that @katis did and mix some of the ideas from @rrousselGit 's functional_widget?

As a user, you write an abstract class [XXX]Base implementing the Store interface. For example...

abstract class CounterBase implements Store {
  @observable 
  int value = 0;
  
  @action
  void increment() {
    value++;
  }
}

and the generator generates...

class Counter extends CounterBase {
  // Rest of the implementation as is
}

So, we introduce the class identifier Counter automatically by virtue of including the part file. Essentially CounterBase → Counter. I think this will eliminate that one line of boilerplate we have to write currently where we create a class with the mixin.

class Counter = CounterBase with _$Counter;

Do you guys see any potential issues with this approach??

[rrousselGit]

Just passing there:

While this works fine, this has other issues that I'm working on:
Refactoring

Renaming CounterBase should rename Counter too. And existing tools to not offer such refactoring yet.

Once we have a support for that in analyzer-plugin then it's fine. But in the mean time, this will cause issues.

[pavanpodila]

Good point! Refactoring will be a pain point. But if you rename CounterBase, the build_runner (running in the background, hopefully) will generate the new class identifier too!

Won't that take care of the problem?

[Update]: I see the problem now. The code that was using Counter won't be updated automatically 😞. Hence we need to explicitly create the class identifier as we have now. Renaming that will automatically rename all usages via the IDE.

[rrousselGit]

The code that was using Counter won't be updated automatically

Indeed.

I took that option for functional_widget because this heavily simplify the syntax to create a widget and I'm fine with making an analyzer plug-in once possible.

But I wouldn't recommend it for mobx

[pavanpodila]

On a separate note, circling back to what @katis asked earlier, can we adopt the convention like so:

class Counter = _CounterBase with _CounterMixin;

abstract class _CounterBase implements Store {}

• Use _XXXBase as the name of the abstract class that implements Store. Since it would never be required outside, we can safely make it private.
• Use _XXXMixin as the name of the mixin. This is similar to other mixins in the SDK like ListMixin, MapMixin, etc. The _$XXX convention felt a bit cryptic (even perl-like 😄).

[katis]

The _$Foo convention is used by other code generation libraries for names that are created by the generator, less chance of name conflicts with user code

[pavanpodila]

Got it. Let's keep it then.

[katis]

One of the benefits of having the user define the class that implements the mixin is that they can add more code to the wrapped class, I wanted to use the InheritedWidget Store.of(context) pattern, so with the current code generation I could do this:

class MyStore extends _MyStore with _$MyStore {
  static MyStore of(BuildContext context) {
    final InjectMyStore widget =
        context.inheritFromWidgetOfExactType(InjectMyStore);
    return widget.myStore;
  }
}

abstract class _MyStore implements Store {
  ...
}

[rrousselGit]

How does renaming fields work in that case BTW?

Since the generator users will use the wrapper class instead of the private base model, it may be possible that renaming fields breaks.

[pavanpodila]

The generated fields and methods retain the same name via @overrides, so we are not introducing any new identifiers into the code.
Refactoring should work fine. I've tried that already :-) and the IDE takes care of updating the *.g.dart files automatically. In fact, you don't even have to run the build_runner!

[pavanpodila]

One of the benefits of having the user define the class that implements the mixin is that they can add more code to the wrapped class, I wanted to use the InheritedWidget Store.of(context) pattern, so with the current code generation I could do this:

class MyStore extends _MyStore with _$MyStore {
  static MyStore of(BuildContext context) {
    final InjectMyStore widget =
        context.inheritFromWidgetOfExactType(InjectMyStore);
    return widget.myStore;
  }
}

abstract class _MyStore implements Store {
  ...
}

If we inject this method into the generated code, you would add a dependency on Flutter. I think there are cases where you will want to keep the Stores as pure data models. Perhaps, have a second interface (InheritedStore or something), which is a hint to the generator to generate the static of() method?

[katis]

of is just user written code, nothing to do with codegen

[pavanpodila]

Ah...now I see it. Thanks for clarifying

[pavanpodila]

Closing this as the generators are in place and working