schultek / stormberry

Access your postgres database effortlessly from dart code.
https://pub.dev/packages/stormberry
MIT License
68 stars 17 forks source link

Use extends instead of implements for view classes #41

Closed abitofevrything closed 1 year ago

abitofevrything commented 1 year ago

Closes #40

schultek commented 1 year ago

This now uses with instead of extends. What was the reason for this?

I think the mixin leads to an error when the model has a constructor. I don't see a disadvantage of extends right now.

abitofevrything commented 1 year ago

I quickly mentioned it in the commit message, but I should have explained better.

As it stands now, we can't use implements because if the models define any methods, we would have to also implement them in the view class, which we don't do. Therefore, we could use extends instead of implements to allow us to inherit that functionality.

This also has an issue: in order to get generators like json_serializable to work, the model classes must declare a constructor that the builder can parse & call. This means that the constructor must also create a new instance, and since model classes are abstract, this means we must define a factory constructor that forwards to one of the view classes' constructors.

This leads to our next issue, which is that adding a single factory constructor removes the default generative constructor, meaning that we can no longer extend our class (no_generative_constructors_in_superclass). This can of course be fixed by adding an empty generative constructor to the model class, but then we might run into issues with name conflicts and the model classes not calling the super constructor...

Using with fixes this behaviour as with does not require the mixin class to have a generative constructor, fixing the issue.

There are however a few drawbacks to using with, but none of them introduce a regression in functionality:

schultek commented 1 year ago

The model classes are actually not required to be abstract. This is just used so you normally don't have to create a constructor for all fields.

However something like this theoretically works just fine as a model:

@jsonSerializable
@Model()
class Account {
  @PrimaryKey()
  @AutoIncrement()
  final int id;

  final String firstName;
  final String lastName;

  Account(this.id, this.firstName, this.lastName);

  // json serializable stuff
  factory Account.fromJson(Map<String, dynamic> json) => _$AccountFromJson(json);

  Map<String, dynamic> toJson() => _$AccountToJson(this);
}

Then when switching to extends, the generated view would have to call the super-constructor. Although in this case there wouldn't even be a need for a separate view class as the code could just use the account class and constructor as is.

abitofevrything commented 1 year ago

This is a fair point. I don't think many users will have non-abstract model classes though, and certainly not non abstract ones with methods as we still then have the issue of them not being implemented in the view class.

schultek commented 1 year ago

Yes currently this is probably not a common issue. However this relates to what I am thinking about for a while now, which is how to easily do serialization for the models, including their views.

The main issue here is when using views, they cannot implement the original model since their fields may change. Then there is currently not a good way to add the required serialization stuff to them (annotations, constructors, etc). But thats out of scope, just to give some context.

I think I can merge you pr now using the mixin way, but be aware this might change either way in the future. But can you add a bit more context to the original issue why you need this and how you want to use this, so I can be aware of that in the future.