Instagram / IGListKit

A data-driven UICollectionView framework for building fast and flexible lists.
https://instagram.github.io/IGListKit/
MIT License
12.87k stars 1.54k forks source link

Is `isEqualToDiffableObject:` necessary #509

Closed PhilCai1993 closed 7 years ago

PhilCai1993 commented 7 years ago

IGListDiffable protocol requires isEqualToDiffableObject: and diffIdentifier.

But I think diffIdentifier is enough to do the diff.

Why do I think isEqualToDiffableObject: is not necessary?

Sometimes it's possible that the implementation of diffIdentifier and isEqualToDiffableObject: is not consistent.

The simplest way is just return "self" for diffIdentifier.

@interface ClassA ()<IGListDiffable>
@interface ClassB ()<IGListDiffable>
...
- (nonnull id<NSObject>)diffIdentifier {
   return self;
}

And both ClassA and ClassB could optionally implement their own -isEqual: and -hash, according to their needs.

As a result, [id<IGListDiffable>instance1.diffIdentifier isEqual:id<IGListDiffable>instance2.diffIdentifier] becomes [instance1 isEqual:instance2].

rnystrom commented 7 years ago

Good question! So we actually didn't use to have isEqualToDiffableObject: at all! We just used isEqual:.

The point of having the two methods has to do with identity and equality, where the diff identifier uniquely identifies data (common scenario is primary key in databases). Equality comes into play when comparing the values of two uniquely identical objects (driving reloading).

For even more history, we originally set out to just use hash and isEqual:, but after @ryanolsonk and I filled up too many whiteboards, we settled on having an object-based identifier.

PhilCai1993 commented 7 years ago

I thought about this because of #503 I guess that crash lies in the inconsistence of the two methods.

jessesquires commented 7 years ago

@rnystrom - should we update the diffable guide with this discussion?

https://instagram.github.io/IGListKit/iglistdiffable-and-equality.html

rnystrom commented 7 years ago

@jessesquires ya lets

jessesquires commented 7 years ago

closing as resolved