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

feat: make Equatable abstract mixin class #161

Closed

Conversation

utamori
Copy link

@utamori utamori commented Jun 2, 2023

Status

READY

Breaking Changes

NO

Description

Make Equatable class an abstract mixin class so that it can be used as a mixin

Related PRs

no

Todos

  • Tests
  • Documentation
  • Examples

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

dart test

Impact to Remaining Code Base

This PR will affect:

  • Probably not.

@utamori utamori requested a review from felangel as a code owner June 2, 2023 12:56
@utamori utamori changed the title feat: make Equatable abstract mixin class feat: make Equatable abstract mixin class Jun 2, 2023
Copy link

@shilangyu shilangyu left a comment

Choose a reason for hiding this comment

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

@felangel what are your thoughts?

repository: https://github.com/felangel/equatable
issue_tracker: https://github.com/felangel/equatable/issues
homepage: https://github.com/felangel/equatable
documentation: https://github.com/felangel/equatable

environment:
sdk: ">=2.12.0 <3.0.0"
sdk: ">=3.0.0 <4.0.0"

Choose a reason for hiding this comment

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

This is a breaking change, so I think we could remove EquatableMixin all together and mark this as release v3?

@@ -18,7 +18,7 @@ import './equatable_utils.dart';
/// ```
/// {@endtemplate}
@immutable
abstract class Equatable {
abstract mixin class Equatable {

Choose a reason for hiding this comment

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

As per my issue, I still doubt that this should be a class at all. This works perfectly fine as a mixin-only, so I see no reason as to why would anyone extend it instead of mixin it in.

Copy link
Owner

Choose a reason for hiding this comment

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

It would be quite inconvenient for existing apps if we forced them to refactor all classes to use equatable as a mixin imo

Choose a reason for hiding this comment

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

fair enough, for sake of compatibility it makes sense. In the long run though (and since it is a breaking change anyways) I don't see value in it being a class.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't explain myself well enough: an abstract mixin class can be extends equally well as an abstract class.
Users can update it without change their code.

image

https://dart.dev/language/modifier-reference

Choose a reason for hiding this comment

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

@felangel even in a large project, this change should be easy to make, a simple search and replace can do 95% of the work, plus people can choose to update the version or not.

Copy link
Owner

@felangel felangel May 26, 2024

Choose a reason for hiding this comment

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

This is still a breaking change because even as a mixin class you won't be able to use Equatable in cases like this:
Screenshot 2024-05-26 at 1 45 23 PM

@felangel
Copy link
Owner

felangel commented May 26, 2024

This change is still a breaking change and I don't think it's worth it especially considering I'm planning to make the next major version of equatable use macros instead so you'd always just annotate your class with @Equatable and that's it (no more props 🎉)

@Equatable()
class Person {
  const Person({required this.name});
  final String name;
}

See also https://github.com/felangel/data_class which will be built on top of the equatable macro so you could go a few steps further:

@Data()
class Person {
  final String name;
}

@felangel
Copy link
Owner

Closing for now based on my above comment, thanks so much for the contribution though -- I really appreciate it!

@felangel felangel closed this May 27, 2024
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.

None yet

4 participants