[ResourceItem] Performance issue on shouldComponentUpdate

[posth]

Issue summary

When using the <ResourceList /> component, and passing a custom child component to the <ResourceList.Item /> that has a loading state attached to an API call, shouldComponentUpdate does some deep comparisons on the props to determine if the item should re-render, which causes a blocking of the rendering of the page for very long periods of time for the specific use-case.

Expected behavior

shouldComponentUpdate optimizes its comparison of possible complex child components.

Deep comparisons in shouldComponentUpdate aren't ideal, see related issue

Actual behavior

Use-case example

This was found in the Shopify web admin, blocking for 13 seconds in this example - locally this comparison is much faster, but still observable.

Performance profile example of the use-case in production

isEqual starts to go very deep in React FiberNodes

Steps to reproduce the problem

This is difficult because the lag is only very noticeable on the production code of Shopify web.

However, here's a sandbox that tries to mimic the same behavior:

https://codesandbox.io/embed/festive-oskar-xq5xz?fontsize=14

The performance profile of that sandbox looking at the isEqual check

Related issue for Shopify admin web is https://github.com/Shopify/store/issues/9546

Removing this props deep equal check from the Shopify web production build resolves this issue.

Specifications

  System:
    OS: macOS 10.14.5
    CPU: (8) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
    Memory: 125.98 MB / 16.00 GB
    Shell: Unknown
  Binaries:
    Node: 10.13.0 - ~/.nvm/versions/node/v10.13.0/bin/node
    Yarn: 1.15.2 - /usr/local/bin/yarn
    npm: 6.4.1 - ~/.nvm/versions/node/v10.13.0/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  Browsers:
    Chrome: 76.0.3809.132
    Firefox: 68.0.2
    Safari: 12.1.1
  npmPackages:
    react: ^16.8.6 => 16.8.6
    react-dom: ^16.8.6 => 16.8.6

[ltakens]

I experience a similar issue. Although on my low-end laptop the browser tab just freezes.
Context: I am trying to sort/rerender a ResourceList in which the ResourceItems have an Avatar media component.

This seems to happen:
From here the props for the resourceitem are compared to the nextProps. The props object for resourceitem has a key called media. In my case the media value holds a property '_owner' that points to a FiberNode. That FiberNode has a property 'return' that points to another FiberNode, whose return property points to another FiberNode... and so on. The lodash comparison functions recursively compare all those. As it says in their code, this may be "susceptible to call stack limits".

If I set the media property of the resourceitem to null, the resourcelsit rerenders without issues.

(FYI call stack start at bottom)

[robin-drexler]

Stumbled upon this again and judging by redux-form/redux-form#3461 (comment), comparing react elements appears to discouraged and we're not the only ones having issues with doing so.

However, there's https://www.npmjs.com/package/react-fast-compare (by formidable labs), which seems to work with react components as well.

I'd suggest that we either use that library in place of isEqual from lodash or remove the children comparisons from polaris.

[robin-drexler]

it seems like the current shouldComponentUpdate implementation in the ResourceItem always returns true because lodash isEqual always seems to return false for the children comparison.
https://codesandbox.io/s/condescending-shannon-85fbz

I tried the exact same example with react-fast-compare where it worked. 🎉

[avocadomayo]

Hi team, I'm having an issue with this exact problem in prod on web.

I have a component called RecentActivitiesList that utilizes ResourceItem in a child component.

Locally, there are no freezing issues. However the freezing problem caused by the issue in shouldComponentUpdate is serious in prod- it blocks the rendering thread for several minutes (see an example profile below)

We have used ResourceItem in the past without any issues- this is the first time I'm observing this issue. Does anyone have any tips on reproducing this locally? Or any insight into getting around this problem? 🙏

[jineshshah36]

Hi, I wanted to post this here as well as I'm not sure if closed pull requests are watched:

Hi, we merged this change it broke a bunch of our ResourceItem components. I have put together a codesandbox here which exemplifies the problem.

Essentially, if you attempt to update the children of the resource item asynchronously without making any additional prop changes, it will never re-render. In the example I included, the author is "fetched" asynchronously, and when we attempt to re-render with the author info, it doesn't.

I believe excluding children is simply a cure for the symptom, but the real problem is that the code is using deep equals instead of shallow equals. Is there a reason for the deep equals? Shouldn't all props be immutable, therefore not need a deep equals comparison? It seems like you could solve all problems by just doing a shallow equals across all props? I would argue that if you need deep equals, it would imply that some of your props are not immutable.