Skip to content

Commit

Permalink
VirtualizedList up-to-date state: Remove _isVirtualizationDisabled()
Browse files Browse the repository at this point in the history
Summary:
This diff is part of an overall stack, meant to fix incorrect usage of `setState()` in `VirtualizedList`, which triggers new invariant checks added in `VirtualizedList_EXPERIMENTAL`. See the stack summary below for more information on the broader change.

## Diff Summary

Remove `this.props` usage during state update where we can just use the props directly.

## Stack Summary
`VirtualizedList`'s component state is a set of cells to render. This state is set via the `setState()` class component API. The main "tick" function `VirtualizedList._updateCellsToRender()` calculates this new state using a combination of the current component state, and instance-local state like maps, measurement caches, etc.

From: https://reactjs.org/docs/state-and-lifecycle.html#state-updates-may-be-asynchronous
 ---
> React may batch multiple setState() calls into a single update for performance. Because this.props and this.state may be updated asynchronously, you should not rely on their values for calculating the next state. For example, this code may fail to update the counter:

```
// Wrong
this.setState({
  counter: this.state.counter + this.props.increment,
});
```
> To fix it, use a second form of setState() that accepts a function rather than an object. That function will receive the previous state as the first argument, and the props at the time the update is applied as the second argument:
```
// Correct
this.setState((state, props) => ({
  counter: state.counter + props.increment
}));
```
 ---
`_updateCellsToRender()` transitively calls many functions which will read directly from `this.props` or `this.state` instead of the value passed by the state updater. This intermittently fires invariant violations, when there is a mismatch.

This diff migrates all usages of `props` and `state` during state update to the values provied in `setState()`. To prevent future mismatch, and to provide better clarity on when it is safe to use `this.props`, `this.state`, I overrode `setState` to fire an invariant violation if it is accessed when it is unsafe to:
{F756963772}

Changelog:
[Internal][Fixed] - Remove _isVirtualizationDisabled()

Reviewed By: genkikondo

Differential Revision: D38293589

fbshipit-source-id: af18f867fc880d5363134fe0d6f985efaf8be6c5
  • Loading branch information
NickGerleman authored and facebook-github-bot committed Aug 1, 2022
1 parent 03b4764 commit cd595bd
Showing 1 changed file with 2 additions and 6 deletions.
8 changes: 2 additions & 6 deletions Libraries/Lists/VirtualizedList_EXPERIMENTAL.js
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,7 @@ class VirtualizedList extends React.PureComponent<Props, State> {
}

let newCellsAroundViewport: {first: number, last: number};
if (this._isVirtualizationDisabled()) {
if (props.disableVirtualization) {
const renderAhead =
distanceFromEnd < onEndReachedThreshold * visibleLength
? maxToRenderPerBatchOrDefault(props.maxToRenderPerBatch)
Expand Down Expand Up @@ -825,10 +825,6 @@ class VirtualizedList extends React.PureComponent<Props, State> {
});
};

_isVirtualizationDisabled(): boolean {
return this.props.disableVirtualization || false;
}

_isNestedWithSameOrientation(): boolean {
const nestedContext = this.context;
return !!(
Expand Down Expand Up @@ -947,7 +943,7 @@ class VirtualizedList extends React.PureComponent<Props, State> {
if (section.isSpacer) {
// Legacy behavior is to avoid spacers when virtualization is
// disabled (including head spacers on initial render).
if (this._isVirtualizationDisabled()) {
if (this.props.disableVirtualization) {
continue;
}

Expand Down

0 comments on commit cd595bd

Please sign in to comment.