Bugfix ensure reference to getStableId is updated #348
Conversation
…hasStableIds and has changed.
DevopsElectricJukebox
changed the title
| this._virtualRenderer = new VirtualRenderer(this._renderStackWhenReady, (offset) => { | ||
| this._pendingScrollToOffset = offset; | ||
| }, (index) => { | ||
| return this.props.dataProvider.getStableId(index); |
There was a problem hiding this comment.
This doesn't seem to do what you expect. This inline function seems to be permanently bound to the first dataProvider.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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.
| @@ -367,9 +370,19 @@ export default class RecyclerListView<P extends RecyclerListViewProps, S extends | |||
| this._params.itemCount = newProps.dataProvider.getSize(); | |||
| this._virtualRenderer.setParamsAndDimensions(this._params, this._layout); | |||
| this._virtualRenderer.setLayoutProvider(newProps.layoutProvider); | |||
| if (newProps.dataProvider.hasStableIds() && this.props.dataProvider !== newProps.dataProvider && newProps.dataProvider.requiresDataChangeHandling()) { | |||
There was a problem hiding this comment.
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 falseor
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?
There was a problem hiding this comment.
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.
|
@DevopsElectricJukebox I'm unable to repro this myself. Can you help me with an expo sample so that I can verify and merge this? |
(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:
This solution is to update the cached function in VirtualRenderer when the DataProvider hasStableIds and has changed.