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

componentDidUpdate forces synchronous layout, which causes layout thrashing #263

Closed
lencioni opened this issue Jul 6, 2018 · 0 comments
Closed

Comments

@lencioni
Copy link
Collaborator

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.

dev vacation rentals homes experiences places - airbnb 2018-07-06 08-16-08

https://github.com/brigade/react-waypoint/blob/b06941c20643afb8a3d427f7c8d79b6ba137f2bb/src/waypoint.jsx#L80-L92

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.

https://github.com/brigade/react-waypoint/blob/b06941c20643afb8a3d427f7c8d79b6ba137f2bb/src/waypoint.jsx#L47-L73

I think we should probably 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}
/>
  1. 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.
lencioni added a commit that referenced this issue 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.

   ```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
lencioni added a commit that referenced this issue 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.

   ```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 added a commit that referenced this issue 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.

   ```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
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

No branches or pull requests

1 participant