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

fix: re-render Background only if a larger image is needed #782

Merged
merged 6 commits into from
Apr 23, 2021

Conversation

ericdeansanchez
Copy link
Contributor

@ericdeansanchez ericdeansanchez commented Apr 19, 2021

The purpose of this PR is to keep as much of the original behavior of the
Background component as possible while curbing the number of times
this component re-renders. Prior, this component re-renders an "infinite"
number of times. Really, it doesn't stop re-rendering.

Now, this component re-renders on a more appropriate interval. Prior,
this component would re-render beginning with the first render. This
means that if you were to render a Background component without
resizing the browser at all it would render a few hundred times in the span
of a couple seconds. Now, there are three initial renders.

This rendering behavior is expected given the behavior of react-measure
in this case
.

Prior (and ignoring the continuous rendering behavior), this component
could re-render if height/width changed. Now, this component will
only be re-rendered if height/width have increased.

Closes #387

@ericdeansanchez ericdeansanchez requested a review from a team as a code owner April 19, 2021 23:08
@commit-lint
Copy link

commit-lint bot commented Apr 19, 2021

Bug Fixes

  • BackgroundImpl: re-render only if a larger image is needed (5275e25)

Code Refactoring

  • component should update if props or imgixParams change (4dd1499)

Chore

Contributors

ericdeansanchez

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@ericdeansanchez ericdeansanchez changed the title fix(BackgroundImpl): re-render only if a larger image is needed fix: re-render Background only if a larger image is needed Apr 19, 2021
@ericdeansanchez ericdeansanchez changed the title fix: re-render Background only if a larger image is needed fix: rerender Background only if a larger image is needed Apr 19, 2021
@ericdeansanchez ericdeansanchez changed the title fix: rerender Background only if a larger image is needed fix: re-render Background only if a larger image is needed Apr 19, 2021
Copy link
Contributor

@frederickfogerty frederickfogerty left a comment

Choose a reason for hiding this comment

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

This is looking good! It seems like the initial implementation really was quite broken.

Something else I was wondering when reading this: does this change the re-rendering behaviour that is ultimately caused by react-measure? i.e. is shouldComponentUpdate continuously called? Or is this actually stopping react-measure from trying to re-render constantly?

super(props);
}

shouldComponentUpdate(nextProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This also needs to handle cases when other props change, e.g. imgixParams, disableLibraryParam, etc.

test/integration/react-imgix.test.jsx Show resolved Hide resolved
@ericdeansanchez
Copy link
Contributor Author

ericdeansanchez commented Apr 21, 2021

Something else I was wondering when reading this: does this change the re-rendering behaviour that is ultimately caused by react-measure? i.e. is shouldComponentUpdate continuously called? Or is this actually stopping react-measure from trying to re-render constantly?

The original behavior is re-rendering occurs from the moment of first-render onward. The component re-renders ~70 times per second (and there's nothing stopping it, this is also "70 times per second without resizing the browser").

The new behavior renders 3 times on first render which is expected given how onResize works for the Measure component (it is called twice on mount).

shouldComponentUpdate is not called continuously. It is called when props have been updated. Specifically, the contectRect changes within the WithContentRect wrapper component, but a re-render only occurs if the width/height within the contentRect have increased.

Now we can count renders: initial + 3 resizes smaller + 3 resizes larger = 7 measures = 21 renders (and no more). If you were to try to collect a similar count on the original behavior it's: 7 measures = 485 renders (more or less depending on how quickly you perform the resizes, again because it's constantly re-rendering.

Visually, this first image is the number of renders with the original component mounting for the first time only. No resizing occurs and I only recorded for 1.3s (e.g. 78th of 78 renders at 1.3s).

70-renders-per-second

This image is of how the updated component renders. I started out with a small browser window and upsized a few times. Here the component actually needs to update; it does so at the 13.4s mark. From this you can see the improvement over rendering 70x/sec.

last-render

And here is the final update, but only to the underlying contentRect:

last-recorded-update-to-contentRect

Now, there is room for improvement here. contentRect is a little noisy wrt to updates from the ResizeObserver entry. That's not a huge deal. Lower hanging fruit would be to only update if height has increased beyond some constant factor of the previous amount (i.e. re-rendering for a height change from 27px to 28px doesn't seem very ideal, but this is all meant to be an iterative start).

@frederickfogerty
Copy link
Contributor

Thanks for the in depth reply here @ericdeansanchez!

imgix-git-robot and others added 3 commits April 22, 2021 23:57
## [9.1.0](v9.0.4...v9.1.0) (2021-04-22)

### Features

* integrate @imgix/js-core into react-imgix ([#780](#780)) ([690e7b6](690e7b6)), closes [#763](#763)

 [skip ci]
Comment on lines +50 to +52
// If we made it here, we need to check if the "top-level"
// props have changed (e.g. disableLibraryParam).
const shallowPropsEqual = shallowEqual(this.props, nextProps);
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be more than we eventually want to check for, but I think it's a good enough starting point for now 👍

@ericdeansanchez ericdeansanchez merged commit 53bb104 into main Apr 23, 2021
@ericdeansanchez ericdeansanchez deleted the e/background branch April 23, 2021 22:45
imgix-git-robot pushed a commit that referenced this pull request Apr 24, 2021
### [9.1.1](v9.1.0...v9.1.1) (2021-04-24)

### Bug Fixes

* re-render Background only if a larger image is needed ([#782](#782)) ([53bb104](53bb104)), closes [#763](#763)

 [skip ci]
@imgix-git-robot
Copy link

🎉 This PR is included in version 9.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

nextProps.imgixParams
);

return shallowPropsEqual && shallowParamsEqual;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great so far! Two things that should be done here:

  1. imgixParams shouldn't be checked on L52
  2. htmlAtrtibutes should also be shallowly compared.

This could be done like this:

    const customizer = (value, other, key) => {
        if (key === "imgixParams" || key === "htmlAttributes") {
            return shallowEqual(value, other);
        }
        return undefined; // Use default behaviour
    };

    // If we made it here, we need to check if the "top-level"
    // props have changed (e.g. disableLibraryParam).
    const shallowPropsEqual = shallowEqual(this.props, nextProps, customizer);

    return shallowPropsEqual;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using <Background /> tanks my CPU
4 participants