List cannot be used safely from function components

[kokuda]

I originally wrote this up as issue https://github.com/microsoft/fluentui/issues/17616 but it was closed with a comment of

Still require assistance? Please, create a new issue with up-to date details.

Describe the issue:

The List component cannot be used consistently from non-class components.

In _getDerivedStateFromProps (old versions used _componentWillReceiveProps) the new props are passed to _updatePages. However, the building process involves other methods, such as _getPageSpecification which uses this.props.getPageSpecification.

If the parent component is a functional component then there is a good chance that the function in this.props.getPageSpecification is not the same function as the new props.getPageSpecification. If its closure contains references to any other props, the function being called will be referencing older props.

Actual behavior:

When the items prop is updated, the previous version of the getPageSpecification prop is called.

Expected behavior:

When any prop is updated, only the latest props are used.

If applicable, please provide a codepen repro:

https://codepen.io/kokuda/pen/qBRayjJ

Priorities and help requested (not applicable if asking question):

Are you willing to submit a PR to fix? (Yes, No)

No

It is unclear how to fix this without a major refactor. Either don't use getDerivedStateFromProps or pass all props to all methods?

Requested priority: (Blocking, High, Normal, Low)

High

I will work around this, but it was difficult to find and should either be well documented or fixed. The component functions behaving in such an unexpected fashion causes some subtle issues and not always exceptions.

The workaround is to use refs within the getPageSpecification implementation so that it always references the current value of the state and not closure captured values. The implementation of the callback getPageSpecification then doesn't depend on closure props or state so it is the same function on each render (like a function component)

[gouttierre]

Thanks for filing this issue with us. To set expectations the developers will review this issue once capacity allows.

@bsunderhus - Would you be able to confirm if this is a regression, or if this behavior is an issue?

[croraf]

Can this manifest itself by having the constructor being called before getDerivedStateFromProps in the parent component?

[layershifter]

@kokuda thanks for the excellent repro, I modified it a bit to highlight the problem: https://codesandbox.io/s/fancy-architecture-6hdezl?file=/src/App.tsx:0-1554

render:i 1 // current value
getPageSpecification:i 0 // is called from `getDerivedStateFromProps()` with older value
getPageSpecification:i 1

The fix that I potentially see is:

  private _getPageSpecification<T>(
+   props: IListProps<T>,
    itemIndex: number,
    visibleRect: IRectangle,
  ): {
    // These return values are now no longer optional.
    itemCount: number;
    height: number;
    data?: any;
    key?: string;
  } {
-    const { getPageSpecification } = this.props;
+    const { getPageSpecification } = props;

Then it does what is expected:

But in general it's hard to say what else will be broken without full refactor ¯_(ツ)_/¯ I tried to use useEventCallback() (https://codesandbox.io/s/intelligent-engelbart-kg4mrr) but it will not work as the component keeps reference.