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

[ericdeansanchez]

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

[commit-lint]

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

• release: 9.1.0 (987dd74)

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

[frederickfogerty]

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?

[frederickfogerty]

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

[ericdeansanchez]

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).

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.

And here is the final update, but only to the underlying 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]

Thanks for the in depth reply here @ericdeansanchez!

[sherwinski]

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

[imgix-git-robot]

🎉 This PR is included in version 9.1.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[frederickfogerty]

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;