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

Extend React.PureComponent instead of React.Component #264

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Conversation

lencioni
Copy link
Collaborator

@lencioni lencioni commented Jul 6, 2018

I was doing some profiling and I noticed that when Waypoint updates, we
call this._handleScroll synchronously, which eventually calls functions
that force layout. Since this happens during render, the layout is
immediately invalidated and then triggered again once rendering is
completed.

We avoid this problem in componentDidMount by deferring this work
until the next tick.

I think we should consider making two changes to Waypoint to help
address this:

  1. Extend React.PureComponent instead of React.Component. In the
    place I saw this problem, we were rendering a waypoint with pure
    props, so the update in this case could be completely avoided by
    using PureComponent.

    <Waypoint
      onEnter={this.onEnterWaypoint}
      scrollableAncestor={this.props.scrollableAncestor}
    />
  2. Defer this._handleScroll in componentDidUpdate using
    onNextTick, like we do in componentDidMount. This will still
    require layout, but it will happen around the same time that the
    browser was going to do layout anyway, avoiding the thrash.

This commit implements the first of these two interventions.

Partly addresses #263

@lencioni lencioni requested review from trotzig and jamesplease July 6, 2018 16:52
I was doing some profiling and I noticed that when Waypoint updates, we
call `this._handleScroll` synchronously, which eventually calls functions
that force layout. Since this happens during render, the layout is
immediately invalidated and then triggered again once rendering is
completed.

We avoid this problem in `componentDidMount` by deferring this work
until the next tick.

I think we should consider making two changes to Waypoint to help
address this:

1. Extend `React.PureComponent` instead of `React.Component`. In the
   place I saw this problem, we were rendering a waypoint with pure
   props, so the update in this case could be completely avoided by
   using PureComponent.

   ```jsx
   <Waypoint
     onEnter={this.onEnterWaypoint}
     scrollableAncestor={this.props.scrollableAncestor}
   />
   ```

2. Defer `this._handleScroll` in `componentDidUpdate` using
   `onNextTick`, like we do in `componentDidMount`. This will still
   require layout, but it will happen around the same time that the
   browser was going to do layout anyway, avoiding the thrash.

This commit implements the first of these two interventions.

Partly addresses #263
@jamesplease
Copy link
Collaborator

image

(Emphasized part is highlighted)

Would this be a breaking change, as all children components of a Waypoint must now also be pure?

@lencioni
Copy link
Collaborator Author

lencioni commented Jul 6, 2018

I don't think so, since passing JSX children to a PureComponent deoptimizes the PureComponent, since JSX is essentially a function call that returns a new instance, so it is never pure.

This might surface existing problems with apps that expect a change in context to be picked up by consumers of that context as a re-render but happen to re-render for some other reason, but that's really a bug in that app's code and not a problem with using pure component here.

@lencioni
Copy link
Collaborator Author

lencioni commented Jul 9, 2018

@jamesplease @trotzig Any more thoughts on this or do you think we are good to merge?

Copy link
Collaborator

@trotzig trotzig left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@lencioni lencioni merged commit 5e3ec79 into master Jul 9, 2018
@lencioni lencioni deleted the pure branch July 9, 2018 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants