-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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 lazyRoot
functionality for next/image
#33933
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
lazyRoot
functionality for next/image
instance = observers.get(idList[index]) | ||
} else { | ||
instance = observers.get(id) | ||
idList.push(id) |
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.
When can we remove from this array? This will grow unbounded as the number of images increases.
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.
That was something that I was going to ask as well! Well let me work on it..
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.
When I think about this problem more, the idea is that we need to know if the root
prop has been seen before at a given rootMargin
.
So what about if we created nested maps? The first could be a WeakMap to root
element and then we can nest the rootMargin
.
// initialize
const observers = new WeakMap<HTMLElement, new Map<string, Observer>>()
// usage
const instance = observers.get(root)?.get(margin)
I think the WeakMap will automatically remove the reference when the DOM node is destroyed.
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.
That would be an elegant solution indeed but the WeakMap can only have an object as a key. Passing an Html element as a key produces runtime error.
However in my latest push I simply implemented removing the entry from the idList just after the corresponding observer is deleted as well. Eventually when everything becomes visible both idList and observers' Map are empty.
Besides, idList would rarely grow too large as that would mean that every Image has a different lazyRoot and lazyBoundary combination.
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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! 🎉
This comment has been minimized.
This comment has been minimized.
Failing test suitesCommit: 73b9cc5
Expand output● Middleware base tests › production mode › should override with rewrite externally correctly
Read more about building and testing Next.js in contributing.md. |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
buildDuration | 15.6s | 15.8s | |
buildDurationCached | 6.1s | 6.1s | |
nodeModulesSize | 359 MB | 359 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.279 | 3.343 | |
/ avg req/sec | 762.34 | 747.78 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.365 | 1.399 | |
/error-in-render avg req/sec | 1831.71 | 1787.48 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42 kB | 42 kB | ✓ |
main-HASH.js gzip | 27.4 kB | 27.4 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71.1 kB | 71.1 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | 11koukou/next.js fixLazyroot | 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 fixLazyroot | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.36 kB | 1.36 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.57 kB | 2.57 kB | ✓ |
head-HASH.js gzip | 350 B | 350 B | ✓ |
hooks-HASH.js gzip | 919 B | 919 B | ✓ |
image-HASH.js gzip | 4.94 kB | 5.01 kB | |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.17 kB | 2.26 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.5 kB | 14.7 kB |
Client Build Manifests Overall decrease ✓
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 458 B | -1 B |
Overall change | 459 B | 458 B | -1 B |
Rendered Page Sizes Overall decrease ✓
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
index.html gzip | 532 B | 532 B | ✓ |
link.html gzip | 546 B | 544 B | -2 B |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | -2 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-f97943edf7ae3dd3.js"],
- "/link": ["static\u002Fchunks\u002Fpages\u002Flink-2c909096541ca18d.js"],
+ "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-9e69c038ec9191f6.js"],
+ "/link": ["static\u002Fchunks\u002Fpages\u002Flink-b932c7479a7c37ca.js"],
"/routerDirect": [
"static\u002Fchunks\u002Fpages\u002FrouterDirect-76232dd6bc335a24.js"
],
Diff for image-HASH.js
@@ -1053,13 +1053,32 @@
if (elements.size === 0) {
observer.disconnect();
observers.delete(id);
+ var index = idList.findIndex(function(obj) {
+ return obj.root === id.root && obj.margin === id.margin;
+ });
+ if (index > -1) {
+ idList.splice(index, 1);
+ }
}
};
}
var observers = new Map();
+ var idList = [];
function createObserver(options) {
- var id = options.rootMargin || "";
- var instance = observers.get(id);
+ var id = {
+ root: options.root || null,
+ margin: options.rootMargin || ""
+ };
+ var existing = idList.find(function(obj) {
+ return obj.root === id.root && obj.margin === id.margin;
+ });
+ var instance;
+ if (existing) {
+ instance = observers.get(existing);
+ } else {
+ instance = observers.get(id);
+ idList.push(id);
+ }
if (instance) {
return instance;
}
Diff for link-HASH.js
@@ -474,13 +474,32 @@
if (elements.size === 0) {
observer.disconnect();
observers.delete(id);
+ var index = idList.findIndex(function(obj) {
+ return obj.root === id.root && obj.margin === id.margin;
+ });
+ if (index > -1) {
+ idList.splice(index, 1);
+ }
}
};
}
var observers = new Map();
+ var idList = [];
function createObserver(options) {
- var id = options.rootMargin || "";
- var instance = observers.get(id);
+ var id = {
+ root: options.root || null,
+ margin: options.rootMargin || ""
+ };
+ var existing = idList.find(function(obj) {
+ return obj.root === id.root && obj.margin === id.margin;
+ });
+ var instance;
+ if (existing) {
+ instance = observers.get(existing);
+ } else {
+ instance = observers.get(id);
+ idList.push(id);
+ }
if (instance) {
return instance;
}
Diff for link.html
@@ -27,7 +27,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/pages/link-2c909096541ca18d.js"
+ src="/_next/static/chunks/pages/link-b932c7479a7c37ca.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 fixLazyroot | Change | |
---|---|---|---|
buildDuration | 19.2s | 19.1s | -84ms |
buildDurationCached | 6.1s | 6.2s | |
nodeModulesSize | 359 MB | 359 MB |
Page Load Tests Overall increase ✓
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.282 | 3.277 | 0 |
/ avg req/sec | 761.76 | 762.81 | +1.05 |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.372 | 1.367 | -0.01 |
/error-in-render avg req/sec | 1822.61 | 1828.31 | +5.7 |
Client Bundles (main, webpack, commons)
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
450.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42.1 kB | 42.1 kB | ✓ |
main-HASH.js gzip | 27.4 kB | 27.4 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 71.2 kB | 71.2 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | 11koukou/next.js fixLazyroot | 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 fixLazyroot | 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.56 kB | 2.56 kB | ✓ |
head-HASH.js gzip | 342 B | 342 B | ✓ |
hooks-HASH.js gzip | 911 B | 911 B | ✓ |
image-HASH.js gzip | 4.97 kB | 5.05 kB | |
index-HASH.js gzip | 256 B | 256 B | ✓ |
link-HASH.js gzip | 2.2 kB | 2.28 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.5 kB | 14.7 kB |
Client Build Manifests Overall increase ⚠️
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 460 B | |
Overall change | 459 B | 460 B |
Rendered Page Sizes
vercel/next.js canary | 11koukou/next.js fixLazyroot | Change | |
---|---|---|---|
index.html gzip | 534 B | 534 B | ✓ |
link.html gzip | 548 B | 548 B | ✓ |
withRouter.html gzip | 528 B | 528 B | ✓ |
Overall change | 1.61 kB | 1.61 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-f97943edf7ae3dd3.js"],
- "/link": ["static\u002Fchunks\u002Fpages\u002Flink-2c909096541ca18d.js"],
+ "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-9e69c038ec9191f6.js"],
+ "/link": ["static\u002Fchunks\u002Fpages\u002Flink-b932c7479a7c37ca.js"],
"/routerDirect": [
"static\u002Fchunks\u002Fpages\u002FrouterDirect-76232dd6bc335a24.js"
],
Diff for image-HASH.js
@@ -1053,13 +1053,32 @@
if (elements.size === 0) {
observer.disconnect();
observers.delete(id);
+ var index = idList.findIndex(function(obj) {
+ return obj.root === id.root && obj.margin === id.margin;
+ });
+ if (index > -1) {
+ idList.splice(index, 1);
+ }
}
};
}
var observers = new Map();
+ var idList = [];
function createObserver(options) {
- var id = options.rootMargin || "";
- var instance = observers.get(id);
+ var id = {
+ root: options.root || null,
+ margin: options.rootMargin || ""
+ };
+ var existing = idList.find(function(obj) {
+ return obj.root === id.root && obj.margin === id.margin;
+ });
+ var instance;
+ if (existing) {
+ instance = observers.get(existing);
+ } else {
+ instance = observers.get(id);
+ idList.push(id);
+ }
if (instance) {
return instance;
}
Diff for link-HASH.js
@@ -474,13 +474,32 @@
if (elements.size === 0) {
observer.disconnect();
observers.delete(id);
+ var index = idList.findIndex(function(obj) {
+ return obj.root === id.root && obj.margin === id.margin;
+ });
+ if (index > -1) {
+ idList.splice(index, 1);
+ }
}
};
}
var observers = new Map();
+ var idList = [];
function createObserver(options) {
- var id = options.rootMargin || "";
- var instance = observers.get(id);
+ var id = {
+ root: options.root || null,
+ margin: options.rootMargin || ""
+ };
+ var existing = idList.find(function(obj) {
+ return obj.root === id.root && obj.margin === id.margin;
+ });
+ var instance;
+ if (existing) {
+ instance = observers.get(existing);
+ } else {
+ instance = observers.get(id);
+ idList.push(id);
+ }
if (instance) {
return instance;
}
Diff for link.html
@@ -27,7 +27,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/pages/link-2c909096541ca18d.js"
+ src="/_next/static/chunks/pages/link-b932c7479a7c37ca.js"
defer=""
></script>
<script src="/_next/static/BUILD_ID/_buildManifest.js" defer=""></script>
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 <steven@ceriously.com>
Bug
fixes #32774
fixes #number
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.