RFC: Remove item-level updates from IGListCollectionContext, require them done in batch updates

[rnystrom]

It is way too easy to execute an item-level update out-of-sync with coalesced batch updates and break things. We've observed this in Instagram. Although rare, at our scale its still a measurable problem.

By item-level updates, I'm specifically talking about:

• Reload items
• Insert items
• Delete items
• Reload the whole section controller

The Problem

The preferred way to use them is to batch update your state (e.g. w/e powers numberOfItems) and item changes:

[self.collectionContext performBatchAnimated:YES updates:^{
  NSMutableArray *models = [self.models mutableCopy];
  [models removeObjectAtIndex:2];
  self.models = models;
  [self.collectionContext deleteInSectionController:self 
    atIndexes:[NSIndexSet indexSetWithIndex:2]];
} completion: nil]

However I've seen lots of examples not-using the batch update block:

NSMutableArray *models = [self.models mutableCopy];
[models removeObjectAtIndex:2];
self.models = models;
[self.collectionContext deleteInSectionController:self 
  atIndexes:[NSIndexSet indexSetWithIndex:2]];

We even do this in one of the examples.

The problem w/ this is that if a batch update is in progress, you might update while the UICollectionViewData (private structure) is changing the state of the UICollectionView. While this is all main-thread affined, the whole operation happens over multiple runloop turns, so it should be treated as an async operation (e.g. racing).

Proposal

I propose we remove all of the item-update operations from IGListCollectionContext so they cannot be done w/out batch updates. Instead, we provide a new IGListBatchUpdateContext object as a parameter to the update block that has all of the above batch update methods.

So now your batch updates would look like:

[self.collectionContext performBatchAnimated:YES updates:^(IGListBatchUpdateContext *context) {
  NSMutableArray *models = [self.models mutableCopy];
  [models removeObjectAtIndex:2];
  self.models = models;
  [context deleteInSectionController:self atIndexes:[NSIndexSet indexSetWithIndex:2]];
} completion: nil]

Considerations

That way the design and compiler forces when we can update. The only thing we can't enforce is when the state changes happen. We can use documentation and examples to try get the message across, but there's nothing stopping someone from doing:

// BAD!
NSMutableArray *models = [self.models mutableCopy];
[models removeObjectAtIndex:2];
self.models = models;

[self.collectionContext performBatchAnimated:YES updates:^(IGListBatchUpdateContext *context) {
  [context deleteInSectionController:self atIndexes:[NSIndexSet indexSetWithIndex:2]];
} completion: nil]

Maybe we could add some fancy asserting or something just before performBatchUpdates: that checks the data source's numberOfSections and numberOfItemsInSection: and throws if the data source and UICV are out of sync?

That might be useful as its own thing. We could pinpoint which section controller is borked.

[jessesquires]

💯

[Adlai-Holler]

Wow this is great! I just started learning IGListKit a couple days ago and was wondering how those two update APIs would play together.

What would it mean for two section controllers to do something like this in the same run loop? Would the updates be coalesced and performed all in one batch?

Tangential but related: if I wanted to build a fully data driven arch (~ #38) would something like this be appropriate?

// Pseudocode
- (void)didUpdateToObject:(id)object {
  self.model = object;
  NSArray *oldItems = self.items;
  self.items = [self.class itemsFromObject:object];
  IGListDiff *diff = …;
  [self.collectionContext applyDiff:diff inSectionController:self];
}

Another tangential point, would performUpdatesAnimated:additionalUpdates:completion: or something similar be more appropriate? It (1) shows that this is the method for updating whether it's batch or no, and (2) indicates that the updates you provide are in addition to the section-level updates from the list adapter.

[heumn]

Maybe we could add some fancy asserting or something just before performBatchUpdates: that checks the data source's numberOfSections and numberOfItemsInSection: and throws if the data source and UICV are out of sync?

Here the consumer of the API is alredy on the path down a slipperly slope of "unkowns errors so if you can help him "do the right thing" early on, this sounds like a good idea.

[rnystrom]

Diff up internally to make this change! 🎉