Semantic-ui-react: Dropdown: componentWillReceiveProps performs deep equal on React component

[FabianFrank]

Bug Report

Steps

Create a Dropdown with content, for example like in https://react.semantic-ui.com/modules/dropdown#usage-item-content. The content must contain a React component, for example:

content: <Header icon='desktop' content='Desktop' subheader='The largest size' />,

Expected Result

App should run normally, display the header, update the dropdown as needed, etc.

Actual Result

When Dropdown's componentWillReceiveProps runs, the app freezes for a minute or more. The issue is in line https://github.com/Semantic-Org/Semantic-UI-React/blob/972b999b7e9f85fb6722a1749c4c121c03e5613d/src/modules/Dropdown/Dropdown.js#L426. The deep equals recurses into the Header object, which has cyclical references (for example nextProps.options[0].content._owner === nextProps.options[0].content._owner.return.child) as well as recursive references through the entire app treee (like _owner._debugOwner._debugOwner._debugOwner... all the way to the root) and thus the app hangs.

The code in question came in via https://github.com/Semantic-Org/Semantic-UI-React/pull/2252, where performance risks of a deep equal were already mentioned.

Version

0.82.0

Testcase

The bug won't reproduce in a small sample app, because in that case lodash.isEqual can traverse the entire app quickly. You'll need a fairly large app to get a freeze of several seconds and it gets worse in debug mode, because then React components have the _debugOwner property set, which can be followed all the way to the root component.

[welcome[bot]]

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you've completed all the fields in the issue template so we can best help.

We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

[layershifter]

This will be not easy to fix, see #2252.

[FabianFrank]

@levithomason @layershifter What about a deep equals that does not recurse into react components, or at least not into their debug properties like _debugOwner, would that be an acceptable compromise?

[levithomason]

We just need to be sure that changes to nested properties within options will be considered a change and update the component.

We could add our own lib util for doing equality checks. It could ignore private React properties. Thoughts?

[FabianFrank]

@levithomason I agree, a falsely skipped property is worse than recursing into one too many. I was thinking of a blacklist approach, i.e. use a heuristic to determine if the current object is a React component (i.e. it has a render function, maybe some other properties or functions) and then blacklist certain properties (maybe only _debugOwner if things are speedy after that) and then see how far that gets us. If people are generally ok with such an approach, I'll try to find time this week to make a PR. Let me know!