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

Defer handleScroll in componentDidUpdate #265

Merged
merged 1 commit into from
Jul 12, 2018
Merged

Defer handleScroll in componentDidUpdate #265

merged 1 commit into from
Jul 12, 2018

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 second of these two interventions.

Fixes #263

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 second of these two interventions.

Fixes #263
@lencioni
Copy link
Collaborator Author

lencioni commented Jul 9, 2018

@trotzig @jamesplease PTAL!

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.

3 participants