fix: don't test for runtimeType equality for object comparison take 2

[devmil]

Status

READY

Breaking Changes

NO

Description

Continuation of #188
As discussed in the referenced PR here an extended PR that adds a couple of tests to make sure that Equatable comparison still works as intended (e.g. different classes with same property values should be different) as well as adding a very far-fetched test-case that simulates non-Equatable classes with custom equality logic.

Related PRs

List related PRs against other branches:

branch	PR
master	link

Todos

• Tests
• Documentation
• Examples

Steps to Test or Reproduce

Have an Equatable class with a field that's type is using a custom equality logic

class CustomEqualityVariant1;
class CustomEqualityVariant2;

final val1 = CustomEqualityVariant1();
final val2 = CustomEqualityVariant2();
val1 == val2

final equatableInstance1 = EquatableClass(val1);
final equatableInstance2 = EquatableClass(val2);

instance1 != instance2 // this was not the case pre 2.0.6

Impact to Remaining Code Base

This PR will affect:

• !behavior change! will increase the set of equal==true combinations compared to 2.0.7 and 2.0.5 (all props that have a different runtimeType but are equal are now treated as equal)
• restores 2.0.5 behavior for Equatable props of type List with different SubClass variants that are equal without sharing the same runtimeType

[felangel]

I took a closer look at v2.0.5 and this was not a breaking change from what I can see (runtimeType was still considered in the equality check). The new test you added fails in v2.0.5 so I don't think we should change this behavior now.

[devmil]

I took a closer look at v2.0.5 and this was not a breaking change from what I can see (runtimeType was still considered in the equality check). The new test you added fails in v2.0.5 so I don't think we should change this behavior now.

@felangel
True. Then I will take a deeper look on why equatable 2.0.6 (and 2.0.7 as well) breaks our code base and create a new PR.

But don't you think that the behavior expressed in the new unit tests is wrong? Regardless of being introduced in 2.0.6 or not?

[devmil]

@felangel
I digged deeper and found out why 2.0.6/2.0.7 breaks our code.
The piece of code is not using the custom equality objects directly but they are in a list.
2.0.5 branched out to the DeepCollectionEquality which internally doesn't do runtimeType checks (because it doesn't need to)

equatable/lib/src/equatable_utils.dart

Line 24 in 2117551

if (!_equality.equals(unit1, unit2)) return false;

2.0.6 replaced that approach with a custom list equality logic that uses the equatable equals logic (that has the runtimeType condition) for each item.

equatable/lib/src/equatable_utils.dart

Line 19 in 49c8604

if (!objectsEquals(a.elementAt(i), b.elementAt(i))) return false;

I added a test that works in this branch as well as on 2.0.5 but fails on 2.0.6/2.0.7 to showcase the scenario

[felangel]

@felangel I digged deeper and found out why 2.0.6/2.0.7 breaks our code. The piece of code is not using the custom equality objects directly but they are in a list. 2.0.5 branched out to the DeepCollectionEquality which internally doesn't do runtimeType checks (because it doesn't need to)

equatable/lib/src/equatable_utils.dart

Line 24 in 2117551

if (!_equality.equals(unit1, unit2)) return false;

2.0.6 replaced that approach with a custom list equality logic that uses the equatable equals logic (that has the runtimeType condition) for each item.

equatable/lib/src/equatable_utils.dart

Line 19 in 49c8604

if (!objectsEquals(a.elementAt(i), b.elementAt(i))) return false;

I added a test that works in this branch as well as on 2.0.5 but fails on 2.0.6/2.0.7 to showcase the scenario

Sorry for the delay (I was off for a lot of the holidays). Will try to get this merged later today or tomorrow 👍

[devmil]

Sorry for the delay (I was off for a lot of the holidays). Will try to get this merged later today or tomorrow 👍

No problem. We have a workaround by now.
I think it makes sense to do a major jump if you decide to take the changes in this PR as the behavior is different.

[felangel]

Sorry for the delay (I was off for a lot of the holidays). Will try to get this merged later today or tomorrow 👍

No problem. We have a workaround by now. I think it makes sense to do a major jump if you decide to take the changes in this PR as the behavior is different.

Yeah need to double check if this issue exists in the v3.0.0-dev release (macros rewrite). If not, we can fix it in the v3.0.0 pre-release 👍

Glad you have a workaround 😄

[OpenSphereSoftware]

are there any updates planned on this ?

[felangel]

are there any updates planned on this ?

Planning to get this merged shortly -- apologies for the delay! I just need to carefully think through the v3.0.0 migration now that macros aren't shipping anymore 😢