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.

Sign up for GitHub

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

Jump to bottom

Fix lazyRoot functionality for next/image #33933

Merged
merged 17 commits into from Feb 7, 2022
Merged

Fix lazyRoot functionality for next/image #33933

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ee0f879
fixed lazyRoot functionality
11koukou Feb 2, 2022
5ce2331
Merge branch 'canary' into fixLazyroot
11koukou Feb 3, 2022
42fcec3
Update packages/next/client/use-intersection.tsx
11koukou Feb 4, 2022
bf847d8
Update packages/next/client/use-intersection.tsx
11koukou Feb 4, 2022
cf70d1b
Update packages/next/client/use-intersection.tsx
11koukou Feb 4, 2022
74f9d2b
Update packages/next/client/use-intersection.tsx
11koukou Feb 4, 2022
021e79c
Update packages/next/client/use-intersection.tsx
11koukou Feb 4, 2022
3d6d309
removed getKeyIndex
11koukou Feb 4, 2022
7d9fb0d
fixed freeing observers' idList
11koukou Feb 5, 2022
8a5c925
Merge branch 'canary' into fixLazyroot
11koukou Feb 5, 2022
c149504
fixed the test
11koukou Feb 5, 2022
10d0758
Merge branch 'fixLazyroot' of https://github.com/11koukou/next.js int…
11koukou Feb 5, 2022
13077c8
Merge branch 'canary' into fixLazyroot
11koukou Feb 6, 2022
31e08a8
Merge branch 'canary' into fixLazyroot
11koukou Feb 6, 2022
25658f1
Merge branch 'canary' into fixLazyroot
11koukou Feb 6, 2022
73b9cc5
Merge branch 'canary' into fixLazyroot
styfle Feb 7, 2022
07f87c2
Merge branch 'canary' into fixLazyroot
styfle Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 29 additions & 4 deletions packages/next/client/use-intersection.tsx
View file
Open in desktop
Expand Up @@ -13,8 +13,12 @@ type UseIntersection = { disabled?: boolean } & UseIntersectionObserverInit & {
rootRef?: React.RefObject<HTMLElement> | null
}
type ObserveCallback = (isVisible: boolean) => void
type Identifier = {
root: Element | Document | null
margin: string
}
type Observer = {
id: string
id: Identifier
observer: IntersectionObserver
elements: Map<Element, ObserveCallback>
}
Expand Down Expand Up @@ -83,14 +87,35 @@ function observe(
if (elements.size === 0) {
observer.disconnect()
observers.delete(id)
let index = idList.findIndex(
(obj) => obj.root === id.root && obj.margin === id.margin
)
if (index > -1) {
idList.splice(index, 1)
}
}
}
}

const observers = new Map<string, Observer>()
const observers = new Map<Identifier, Observer>()

const idList: Identifier[] = []

function createObserver(options: UseIntersectionObserverInit): Observer {
const id = options.rootMargin || ''
let instance = observers.get(id)
const id = {
root: options.root || null,
margin: options.rootMargin || '',
}
let existing = idList.find(
(obj) => obj.root === id.root && obj.margin === id.margin
)
let instance
if (existing) {
instance = observers.get(existing)
} else {
instance = observers.get(id)
idList.push(id)
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

@11koukou 11koukou Feb 5, 2022 •
edited

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.

}
if (instance) {
return instance
}
Expand Down
32 changes: 0 additions & 32 deletions test/integration/image-component/default/pages/lazy-noref.js
View file
Open in desktop

This file was deleted.

32 changes: 30 additions & 2 deletions test/integration/image-component/default/pages/lazy-withref.js
View file
Open in desktop
Expand Up @@ -18,13 +18,41 @@ const Page = () => {
<div style={{ width: '400px', height: '600px' }}>hello</div>
<div style={{ width: '400px', position: 'relative', height: '600px' }}>
<Image
lazyRoot={myRef}
id="myImage"
id="myImage1"
src="/test.jpg"
alt="mine"
width="400"
height="400"
lazyBoundary="1500px"
/>
<Image
lazyRoot={myRef}
id="myImage2"
src="/test.png"
alt="mine"
width="400"
height="400"
lazyBoundary="1800px"
/>
<Image
lazyRoot={myRef}
id="myImage3"
src="/test.svg"
alt="mine"
width="400"
height="400"
lazyBoundary="1800px"
/>

<Image
lazyRoot={myRef}
id="myImage4"
src="/test.webp"
alt="mine"
width="400"
height="400"
lazyBoundary="200px"
/>
</div>
</div>
</>
Expand Down
58 changes: 55 additions & 3 deletions test/integration/image-component/default/test/index.test.js
View file
Open in desktop
Expand Up @@ -1010,15 +1010,49 @@ function runTests(mode) {
}
})

it('should load the image when the lazyRoot prop is used', async () => {
it('should initially load only two of four images using lazyroot', async () => {
let browser
try {
//trying on '/lazy-noref' it fails
browser = await webdriver(appPort, '/lazy-withref')
await check(async () => {
const result = await browser.eval(
`document.getElementById('myImage1').naturalWidth`
)

if (result >= 400) {
throw new Error('Incorrectly loaded image')
}

return 'result-correct'
}, /result-correct/)

await check(async () => {
const result = await browser.eval(
`document.getElementById('myImage4').naturalWidth`
)

if (result >= 400) {
throw new Error('Incorrectly loaded image')
}

return 'result-correct'
}, /result-correct/)

await check(async () => {
const result = await browser.eval(
`document.getElementById('myImage').naturalWidth`
`document.getElementById('myImage2').naturalWidth`
11koukou marked this conversation as resolved.
Show resolved Hide resolved
)

if (result < 400) {
throw new Error('Incorrectly loaded image')
}

return 'result-correct'
}, /result-correct/)

await check(async () => {
const result = await browser.eval(
`document.getElementById('myImage3').naturalWidth`
)

if (result < 400) {
Expand All @@ -1033,7 +1067,25 @@ function runTests(mode) {
browser,
`http://localhost:${appPort}/_next/image?url=%2Ftest.jpg&w=828&q=75`
)
).toBe(false)
expect(
await hasImageMatchingUrl(
browser,
`http://localhost:${appPort}/_next/image?url=%2Ftest.png&w=828&q=75`
)
).toBe(true)
expect(
await hasImageMatchingUrl(
browser,
`http://localhost:${appPort}/_next/image?url=%2Ftest.svg&w=828&q=75`
)
).toBe(true)
expect(
await hasImageMatchingUrl(
browser,
`http://localhost:${appPort}/_next/image?url=%2Ftest.webp&w=828&q=75`
)
).toBe(false)
} finally {
if (browser) {
await browser.close()
Expand Down