Skip to content
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

Use extends instead of implements for view classes #41

Merged
merged 5 commits into from
Jan 15, 2023

Conversation

abitofevrything
Copy link
Contributor

Closes #40

@schultek
Copy link
Owner

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
Copy link
Contributor Author

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:

  • Model classes can no longer extend other classes (causes mixin_inherits_from_not_object). Solution: simply use implements instead of extends. No functionality is lost as methods that would have had functionality inherited into the model class via extends is currently removed in the view class because it uses implements anyways.
  • Model classes can also no longer use mixins on themselves, for the same reasons as above. The solution is the same as above.

@schultek
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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.

@schultek schultek merged commit 8d6934f into schultek:develop Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants