Bugfix ensure reference to getStableId is updated

[DevopsElectricJukebox]

(This PR replaces #339)

This is a bugfix for the case when the DataProvider is changed, and may provide entirely different data. The VirtualRenderer needs to update its own reference to getStableId to ensure that it uses the correct IDs for the components it creates.

You can test this bug by the following steps:

1. Instantiate a RecyclerListView with a DataProvider containing 1 item.
2. Change the DataProvider to one containing 3 items
3. The RecyclerListView will crash due to trying to look up the 2 new items using the old DataProvider's getStableId function.

This solution is to update the cached function in VirtualRenderer when the DataProvider hasStableIds and has changed.

[DevopsElectricJukebox]

This doesn't seem to do what you expect. This inline function seems to be permanently bound to the first dataProvider.

[kwketh]

I think another way to look at this is stableId inside virtualRenderer is updated regardless if the inline method permanently bound it or not, therefore the inline method has no effect. I agree this change is 👍

[naqvitalha]

I think the problem here is the fact that look up will happen in old dataProvider till react lifecycle is completed. Only post that this.props will point to the new one.

[kwketh]

I created a small snippet to verify that this change is necessary.

On this line 370, requiresDataChangeHandling() value returns false (but should be true) when:

new DataProvider((a, b) => a !== b)
  .cloneWithRows([1, 2])
  .cloneWithRows([1, 2, 3])
  .requiresDataChangeHandling() // returns false

or

new DataProvider((a, b) => a !== b)
    .cloneWithRows([])
    .cloneWithRows([1, 2, 3])
    .requiresDataChangeHandling() // returns false

(where result of first clone is the initial data provider and second is the next data provider)

In both instances, handleDataSetChange it not called and virtual renderer will continue using stable id method from the previous data provider, and so getStableId(2) would be out of bounds.

This potentially means another patch is required inside cloneWithRows implementation, however regardless of where the bug originated from, I agree that updating stableId method is always necessary when data provider has changed.

I could have misunderstood the purpose of requiresDataChangeHandling, but I assume cloning data provider with new items inserted, it should return true and end up calling handleDataSetChange?

[naqvitalha]

requiresDataChangeHandling is an optimization to avoid managing recycling keys in case of pagination where items are just getting added and no modification is there in existing ones.

[naqvitalha]

@DevopsElectricJukebox I'm unable to repro this myself. Can you help me with an expo sample so that I can verify and merge this?