-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Add lazyRoot
optional property to next/image
component
#33290
Conversation
…root' option of the Intersection Observer API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
A few changes:
- We should automatically determine the intersection root instead of making the user provide a value. I think this can be achieved by recursively walking up the DOM to find the first scrollable element.
- If we do expose this prop on the Image component:
- it should by a ref instead of element
- it should be called
lazyRoot
to match the existinglazyBoundary
- it doesn't need comments
Hello and thanks for the reply too! I experimented with your first approach (the automatic determination) but it seems that there is no safe rule about determining the correct overflowing container. Although unlikely, it is still possible the Image being nested in multiple overflowing containers. Even if I consider that the root container is the first overflowing parent with height greater than the image, it would stil seem arbitrary and unsafe. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
We also need an integration test to ensure this is working correctly.
This comment has been minimized.
This comment has been minimized.
Thank you again as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks! 🎉
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
buildDuration | 12.6s | 12.6s | |
buildDurationCached | 3.2s | 3.3s | |
nodeModulesSize | 356 MB | 356 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 2.917 | 2.977 | |
/ avg req/sec | 856.98 | 839.84 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.383 | 1.378 | -0.01 |
/error-in-render avg req/sec | 1807.81 | 1814.83 | +7.02 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.2 kB | 42.2 kB | ✓ |
main-HASH.js gzip | 27.2 kB | 27.2 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71 kB | 71 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages Overall increase ⚠️
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.37 kB | 1.37 kB | ✓ |
_error-HASH.js gzip | 194 B | 194 B | ✓ |
amp-HASH.js gzip | 312 B | 312 B | ✓ |
css-HASH.js gzip | 326 B | 326 B | ✓ |
dynamic-HASH.js gzip | 2.37 kB | 2.37 kB | ✓ |
head-HASH.js gzip | 350 B | 350 B | ✓ |
hooks-HASH.js gzip | 919 B | 919 B | ✓ |
image-HASH.js gzip | 4.87 kB | 4.94 kB | |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.13 kB | 2.19 kB | |
routerDirect..HASH.js gzip | 321 B | 321 B | ✓ |
script-HASH.js gzip | 383 B | 383 B | ✓ |
withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 14.2 kB | 14.4 kB |
Client Build Manifests Overall increase ⚠️
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 460 B | |
Overall change | 459 B | 460 B |
Rendered Page Sizes Overall increase ⚠️
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
index.html gzip | 531 B | 531 B | ✓ |
link.html gzip | 544 B | 545 B | |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB |
Diffs
Diff for _buildManifest.js
@@ -12,8 +12,8 @@ self.__BUILD_MANIFEST = {
],
"/head": ["static\u002Fchunks\u002Fpages\u002Fhead-7100d3b2a548f0e4.js"],
"/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-538d621a0e670391.js"],
- "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-7c12d79b5adf878e.js"],
- "/link": ["static\u002Fchunks\u002Fpages\u002Flink-1339957a75cc3c85.js"],
+ "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-b1f110951983876f.js"],
+ "/link": ["static\u002Fchunks\u002Fpages\u002Flink-f0a2c3bb0706d8b2.js"],
"/routerDirect": [
"static\u002Fchunks\u002Fpages\u002FrouterDirect-76232dd6bc335a24.js"
],
Diff for image-HASH.js
@@ -480,6 +480,8 @@
_priority = _param.priority,
priority = _priority === void 0 ? false : _priority,
loading = _param.loading,
+ _lazyRoot = _param.lazyRoot,
+ lazyRoot = _lazyRoot === void 0 ? null : _lazyRoot,
_lazyBoundary = _param.lazyBoundary,
lazyBoundary = _lazyBoundary === void 0 ? "200px" : _lazyBoundary,
className = _param.className,
@@ -500,6 +502,7 @@
"unoptimized",
"priority",
"loading",
+ "lazyRoot",
"lazyBoundary",
"className",
"quality",
@@ -570,6 +573,7 @@
}
var ref3 = _slicedToArray(
(0, _useIntersection).useIntersection({
+ rootRef: lazyRoot,
rootMargin: lazyBoundary,
disabled: !isLazy
}),
@@ -978,13 +982,20 @@
var _requestIdleCallback = __webpack_require__(9311);
var hasIntersectionObserver = typeof IntersectionObserver !== "undefined";
function useIntersection(param) {
- var rootMargin = param.rootMargin,
+ var rootRef = param.rootRef,
+ rootMargin = param.rootMargin,
disabled = param.disabled;
var isDisabled = disabled || !hasIntersectionObserver;
var unobserve = (0, _react).useRef();
var ref = _slicedToArray((0, _react).useState(false), 2),
visible = ref[0],
setVisible = ref[1];
+ var ref1 = _slicedToArray(
+ (0, _react).useState(rootRef ? rootRef.current : null),
+ 2
+ ),
+ root = ref1[0],
+ setRoot = ref1[1];
var setRef = (0, _react).useCallback(
function(el) {
if (unobserve.current) {
@@ -999,12 +1010,13 @@
return isVisible && setVisible(isVisible);
},
{
+ root: root,
rootMargin: rootMargin
}
);
}
},
- [isDisabled, rootMargin, visible]
+ [isDisabled, root, rootMargin, visible]
);
(0, _react).useEffect(
function() {
@@ -1024,6 +1036,12 @@
},
[visible]
);
+ (0, _react).useEffect(
+ function() {
+ if (rootRef) setRoot(rootRef.current);
+ },
+ [rootRef]
+ );
return [setRef, visible];
}
function observe(element, callback, options) {
Diff for link-HASH.js
@@ -398,13 +398,20 @@
var _requestIdleCallback = __webpack_require__(9311);
var hasIntersectionObserver = typeof IntersectionObserver !== "undefined";
function useIntersection(param) {
- var rootMargin = param.rootMargin,
+ var rootRef = param.rootRef,
+ rootMargin = param.rootMargin,
disabled = param.disabled;
var isDisabled = disabled || !hasIntersectionObserver;
var unobserve = (0, _react).useRef();
var ref = _slicedToArray((0, _react).useState(false), 2),
visible = ref[0],
setVisible = ref[1];
+ var ref1 = _slicedToArray(
+ (0, _react).useState(rootRef ? rootRef.current : null),
+ 2
+ ),
+ root = ref1[0],
+ setRoot = ref1[1];
var setRef = (0, _react).useCallback(
function(el) {
if (unobserve.current) {
@@ -419,12 +426,13 @@
return isVisible && setVisible(isVisible);
},
{
+ root: root,
rootMargin: rootMargin
}
);
}
},
- [isDisabled, rootMargin, visible]
+ [isDisabled, root, rootMargin, visible]
);
(0, _react).useEffect(
function() {
@@ -444,6 +452,12 @@
},
[visible]
);
+ (0, _react).useEffect(
+ function() {
+ if (rootRef) setRoot(rootRef.current);
+ },
+ [rootRef]
+ );
return [setRef, visible];
}
function observe(element, callback, options) {
Diff for link.html
@@ -27,7 +27,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/pages/link-1339957a75cc3c85.js"
+ src="/_next/static/chunks/pages/link-f0a2c3bb0706d8b2.js"
defer=""
></script>
<script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>
Default Build with SWC (Increase detected ⚠️ )
General Overall increase ⚠️
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
buildDuration | 15.9s | 16.1s | |
buildDurationCached | 3.2s | 3.2s | -5ms |
nodeModulesSize | 356 MB | 356 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 2.909 | 3.021 | |
/ avg req/sec | 859.36 | 827.61 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.378 | 1.407 | |
/error-in-render avg req/sec | 1813.8 | 1776.32 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
main-HASH.js gzip | 27.3 kB | 27.3 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71.3 kB | 71.3 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages Overall increase ⚠️
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 180 B | 180 B | ✓ |
amp-HASH.js gzip | 305 B | 305 B | ✓ |
css-HASH.js gzip | 321 B | 321 B | ✓ |
dynamic-HASH.js gzip | 2.36 kB | 2.36 kB | ✓ |
head-HASH.js gzip | 342 B | 342 B | ✓ |
hooks-HASH.js gzip | 911 B | 911 B | ✓ |
image-HASH.js gzip | 4.89 kB | 4.97 kB | |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 2.19 kB | 2.21 kB | |
routerDirect..HASH.js gzip | 314 B | 314 B | ✓ |
script-HASH.js gzip | 375 B | 375 B | ✓ |
withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 14.2 kB | 14.3 kB |
Client Build Manifests
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 459 B | ✓ |
Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes Overall decrease ✓
vercel/next.js canary | 11koukou/next.js koukou | Change | |
---|---|---|---|
index.html gzip | 530 B | 530 B | ✓ |
link.html gzip | 544 B | 543 B | -1 B |
withRouter.html gzip | 524 B | 524 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | -1 B |
Diffs
Diff for _buildManifest.js
@@ -12,8 +12,8 @@ self.__BUILD_MANIFEST = {
],
"/head": ["static\u002Fchunks\u002Fpages\u002Fhead-7100d3b2a548f0e4.js"],
"/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-538d621a0e670391.js"],
- "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-7c12d79b5adf878e.js"],
- "/link": ["static\u002Fchunks\u002Fpages\u002Flink-1339957a75cc3c85.js"],
+ "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-b1f110951983876f.js"],
+ "/link": ["static\u002Fchunks\u002Fpages\u002Flink-f0a2c3bb0706d8b2.js"],
"/routerDirect": [
"static\u002Fchunks\u002Fpages\u002FrouterDirect-76232dd6bc335a24.js"
],
Diff for image-HASH.js
@@ -480,6 +480,8 @@
_priority = _param.priority,
priority = _priority === void 0 ? false : _priority,
loading = _param.loading,
+ _lazyRoot = _param.lazyRoot,
+ lazyRoot = _lazyRoot === void 0 ? null : _lazyRoot,
_lazyBoundary = _param.lazyBoundary,
lazyBoundary = _lazyBoundary === void 0 ? "200px" : _lazyBoundary,
className = _param.className,
@@ -500,6 +502,7 @@
"unoptimized",
"priority",
"loading",
+ "lazyRoot",
"lazyBoundary",
"className",
"quality",
@@ -570,6 +573,7 @@
}
var ref3 = _slicedToArray(
(0, _useIntersection).useIntersection({
+ rootRef: lazyRoot,
rootMargin: lazyBoundary,
disabled: !isLazy
}),
@@ -978,13 +982,20 @@
var _requestIdleCallback = __webpack_require__(9311);
var hasIntersectionObserver = typeof IntersectionObserver !== "undefined";
function useIntersection(param) {
- var rootMargin = param.rootMargin,
+ var rootRef = param.rootRef,
+ rootMargin = param.rootMargin,
disabled = param.disabled;
var isDisabled = disabled || !hasIntersectionObserver;
var unobserve = (0, _react).useRef();
var ref = _slicedToArray((0, _react).useState(false), 2),
visible = ref[0],
setVisible = ref[1];
+ var ref1 = _slicedToArray(
+ (0, _react).useState(rootRef ? rootRef.current : null),
+ 2
+ ),
+ root = ref1[0],
+ setRoot = ref1[1];
var setRef = (0, _react).useCallback(
function(el) {
if (unobserve.current) {
@@ -999,12 +1010,13 @@
return isVisible && setVisible(isVisible);
},
{
+ root: root,
rootMargin: rootMargin
}
);
}
},
- [isDisabled, rootMargin, visible]
+ [isDisabled, root, rootMargin, visible]
);
(0, _react).useEffect(
function() {
@@ -1024,6 +1036,12 @@
},
[visible]
);
+ (0, _react).useEffect(
+ function() {
+ if (rootRef) setRoot(rootRef.current);
+ },
+ [rootRef]
+ );
return [setRef, visible];
}
function observe(element, callback, options) {
Diff for link-HASH.js
@@ -398,13 +398,20 @@
var _requestIdleCallback = __webpack_require__(9311);
var hasIntersectionObserver = typeof IntersectionObserver !== "undefined";
function useIntersection(param) {
- var rootMargin = param.rootMargin,
+ var rootRef = param.rootRef,
+ rootMargin = param.rootMargin,
disabled = param.disabled;
var isDisabled = disabled || !hasIntersectionObserver;
var unobserve = (0, _react).useRef();
var ref = _slicedToArray((0, _react).useState(false), 2),
visible = ref[0],
setVisible = ref[1];
+ var ref1 = _slicedToArray(
+ (0, _react).useState(rootRef ? rootRef.current : null),
+ 2
+ ),
+ root = ref1[0],
+ setRoot = ref1[1];
var setRef = (0, _react).useCallback(
function(el) {
if (unobserve.current) {
@@ -419,12 +426,13 @@
return isVisible && setVisible(isVisible);
},
{
+ root: root,
rootMargin: rootMargin
}
);
}
},
- [isDisabled, rootMargin, visible]
+ [isDisabled, root, rootMargin, visible]
);
(0, _react).useEffect(
function() {
@@ -444,6 +452,12 @@
},
[visible]
);
+ (0, _react).useEffect(
+ function() {
+ if (rootRef) setRoot(rootRef.current);
+ },
+ [rootRef]
+ );
return [setRef, visible];
}
function observe(element, callback, options) {
Diff for link.html
@@ -27,7 +27,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/pages/link-1339957a75cc3c85.js"
+ src="/_next/static/chunks/pages/link-f0a2c3bb0706d8b2.js"
defer=""
></script>
<script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>
Not sure if I am using lazyRoot incorrectly but it appears as if you use lazyRoot that the images will always immediately load (whether your scroll position is near or not). Here is a more detailed explanation and reproduction: #32774 (comment) |
He
Hello, I can confirm that indeed there is an issue with lazyloading in the latest version. However it seems that it's not relative to the "lazyroot" addition since I can reproduce this bug without defining the lazyroot prop at all, even if I remove completely the code of this PR. It is something that I had noticed as well after #33474 PR however didn't pay much attention since I thought it was my next cache not being cleared. |
I've been trying all day to fix this if possible but with no luck. Observations: When lazyRoot prop is used all the images indeed load at once no matter what lazyBoundary is set (just like setting loading='eager'). Digging down the code it seems that my latest addition of useEffect in the useIntersection.tsx file causes that. However things change when the test page contains many thumbnail images and there even without the lazyRoot property, loading is unpredictable and many times again everything is being loaded at once (eagerly). I confirm again that everything works fine if the code from this PR is added in any version prior to #33474 commit. Cross-checked with multiple tests in various pages and properties. |
…ercel#33290)" This reverts commit 7452c0b.
Ok the issue where everything loads eagerly has dissappeared. However the lazyRoot prop is not working, but I've figured out why. It's because every IntersectionObserver instance is assigned the "lazyBoundary" (rootMargin) prop as an id to be mapped (inside createObserver function of the useIntersection.tsx file). Now it also needs the root option in order to contruct a unique id from these two values. That's a bit tricky because while rootMargin is simply a string, the root is an element and can not be stringified or something. I'm working on it. In the meantime I wonder how it worked previously but nevertheless this is the right way. Could you please tell me how to post when it's ready? Open a new pull request or re-open this one somehow? I mistakenly pressed the "revert" button above and I don't know how to proceed. |
Agreed, seems like it wouldn't have worked before since that means the key would have been the same for every image 🤔
Yep, create a new branch from canary, create a new PR, add a test, thanks! |
Fixed lazyRoot functionality (#33290). Changed the unique id for Intersection Observers discrimination since previously they were only identified by the different rootMargins, now each being identified by the rootMargin and the root element as well Added more Images to the test with different margins and with/without lazyRoot prop. Browser correctly initially loading two of the four Images according to the props' specifications. Co-authored-by: Steven <[email protected]>
) * Added 'rootEl' oprional property to next/Image component resembling 'root' option of the Intersection Observer API * changed 'rootEl' to 'lazyBoundary' and its type as well * added test, fixed initial root detection * Update test/integration/image-component/default/test/index.test.js Co-authored-by: Steven <[email protected]> * prop names changed * added 'lazyroot' prop to the documentation * removed unused import * Apply suggestions from code review * Update docs with lazyRoot added in 12.0.9 Co-authored-by: Steven <[email protected]>
Fixed lazyRoot functionality (vercel#33290). Changed the unique id for Intersection Observers discrimination since previously they were only identified by the different rootMargins, now each being identified by the rootMargin and the root element as well Added more Images to the test with different margins and with/without lazyRoot prop. Browser correctly initially loading two of the four Images according to the props' specifications. Co-authored-by: Steven <[email protected]>
Bug
#33125
Feature
#33125
Documentation / Examples
"Image" component from 'next/image' has now a new optional property called 'lazyRoot' of type HTMLElement. This is for being able to resemble the 'root' option of the Intersection Observer API, just like the existing 'lazyBoundary' option resembles the 'rootMargin' option. Omitting the property things will work as previously (the IO API root option will point to the 'root' Html Element). Since the prop's type is 'React.RefObject' it is recommended to assign the prop to the ref previously created by 'React.createRef()' (for class components) or 'useRef()' for functional components (see usage Example below)
That being done, we are now able to scroll any container we create (with Image components inside it) and being able to calibrate the lazyLoading with 'lazyRoot' and 'lazyBoundary' props accoring to our needs, something that previously only happened when the overflowed container was the root Html Element, as being addressed by the issue mentioned above.
Example Usage:
...
this.rootRef=React.createRef();
...
return
(
< div ref={this.rootRef} >
< Image lazyRoot={this.rootRef} src={''} lazyBoundary='500px' width='50' height='50' alt={''} / >
< /div >
)
yarn lint