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

Framework: Replace is-equal-shallow with shallowequal #5616

Merged
merged 1 commit into from
Mar 14, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 14, 2018

This pull request seeks to resolve a performance regression caused by a bailout condition in our chosen shallow-equality library is-equal-shallow:

    if (!isPrimitive(b[key]) || !a.hasOwnProperty(key) || (a[key] !== b[key])) {
      return false;
    }

https://github.com/jonschlinkert/is-equal-shallow/blob/592a07eb802e8043493a983eee901e2b56b4b05c/index.js#L19-L20

Specifically, our data withSelect higher-order component relies on reference equality check to prevent unnecessary re-renders.

gutenberg/data/index.js

Lines 214 to 218 in 409ac98

if ( ! isEqualShallow( nextMergeProps, mergeProps ) ) {
this.setState( {
mergeProps: nextMergeProps,
} );
}

Since the above logic will abort (and flag as unequal) if it encounters a non-primitive type, this optimization is very frequently bypassed.

The changes here swap to an alternate implementation which does not include such a bailout.

See implementation: https://github.com/dashed/shallowequal/blob/master/index.js

Testing instructions:

With React DevTools, verify that updating a block should only impact the block itself, not re-rendering large amounts of the application.

@aduth aduth added the [Type] Performance Related to performance efforts label Mar 14, 2018
@aduth aduth added this to the 2.4 milestone Mar 14, 2018
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 🚢 🚢 🚢 🚢

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change reduces a lot the number of rerenders 👍 . The insert button still rerenders on every change e.g: every paragraph char change. But this is something to investigate and outside of the scope of this PR.

@mcsf mcsf merged commit 20a13c4 into master Mar 14, 2018
@mcsf mcsf deleted the replace/shallow-equal branch March 14, 2018 17:37
@mcsf mcsf mentioned this pull request Mar 15, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants