Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: use lodash.isEqual to compare react elemet can cause infinite loop #19811

Closed
migodev42 opened this issue Sep 11, 2020 · 5 comments
Closed

Comments

@migodev42
Copy link

React version: v16.13.1

Steps To Reproduce

1.Use lodash.isEqual to compare two object, some of their key`s value are react element
2.Causing inifite loop in dev
bug

Howerver when I use fast-deep-equal React-specific isEqual, it won`t cause this infinite loop.

Screenshot from 2020-09-11 11-14-37

Since I find out React would add a _owner property on dev, is my problem related to this?
Screenshot from 2020-09-11 15-02-10

Link to code example:

The current behavior

The expected behavior

@migodev42 migodev42 added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Sep 11, 2020
@hosseinmd
Copy link

I don't think you should use isEqual for components which have children props.

In a some situation the children components are not memo components, but if wrapped with a component that uses fast-deep-equal that forces it to be a memo component. This way causes them to be memo too. Because memorization going to deep of children too.

@eps1lon
Copy link
Collaborator

eps1lon commented Sep 11, 2020

Cyclic references in JS objects are valid. Your "is equal" implementation should guard against it. This is a problem with lodash's isEqual and not a bug in React. Seems like fast-deep-equal has a fix for that issue. Either use fast-deep-equal or find another workaround for this issue. You can find helpful resources on https://reactjs.org/community/support.html

@eps1lon eps1lon closed this as completed Sep 11, 2020
@eps1lon eps1lon added Resolution: Support Redirect and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Sep 11, 2020
@gaearon
Copy link
Collaborator

gaearon commented Sep 11, 2020

I should note it is a really, really bad idea to pass a React element to isEqual. You’re going to spend a lot of unnecessary time traversing React internals even if you had a protection.

Ideally you should compare elements as shallowly as possible, and instead memoize them where they get created (if needed for perf). As the last resort you may compare element fields except for the ones starting with underscore. But again, this is usually a very bad idea because you’re going to have completely unpredictable performance.

@eps1lon
Copy link
Collaborator

eps1lon commented Sep 11, 2020

Might be worth adding a warning to the docs for custom arePropsEqual implementations (under https://reactjs.org/docs/react-api.html#reactmemo). People might naively reach for some deep equal implementation when trying to memoize props with shallow objects. That works™ until you pass a React element which should be somewhat common.

@danielabyan
Copy link

React components have very heavy keys. When comparing properties, it doesn't make sense to compare these keys because they don't contain application state data. In addition, there is a danger of falling into the circular references trap, which will lead to an endless cycle of validating the properties of the React component. For this reason, these properties are best ignored. This trick is used in the epoberezkin/fast-deep-equal library.

const isPropsEqual = _.isEqualWith(
      data1,
      data2,
      (data1Value, data2Value, key) => {
        const ignoreKeysList = [
          '_owner',
          '$$typeof'
        ]
        return ignoreKeysList.includes(key) ? true : undefined
      }
    )

ivan-nejezchleb added a commit to ivan-nejezchleb/gooddata-ui-sdk that referenced this issue Nov 16, 2022
Comparing whole props including children by lodash/isEqual may end up in infinite loop
facebook/react#19811

It happened when UI tested by Cypress tests

JIRA: TNT-1164
ivan-nejezchleb added a commit to ivan-nejezchleb/gooddata-ui-sdk that referenced this issue Nov 16, 2022
Comparing whole props including children by lodash/isEqual
may end up in infinite loop.
facebook/react#19811

It happened when UI tested by Cypress tests

JIRA: TNT-1164
ivan-nejezchleb added a commit to ivan-nejezchleb/gooddata-ui-sdk that referenced this issue Nov 16, 2022
Comparing whole props including children by lodash/isEqual
may end up in infinite loop.
facebook/react#19811

It happened when UI tested by Cypress tests

JIRA: TNT-1164
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants