Skip to content

Commit 70d9876

Browse files
eps1loneps1lon
authored
[next/image] Only fire onError once (#93209)
1 parent a2fd4ea commit 70d9876

7 files changed

Lines changed: 303 additions & 22 deletions

File tree

‎packages/next/src/client/image-component.tsx‎

Lines changed: 40 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,12 @@
33
import React, {
44
useRef,
55
useEffect,
6-
useCallback,
76
useContext,
87
useMemo,
98
useState,
109
forwardRef,
1110
use,
11+
useLayoutEffect,
1212
} from 'react'
1313
import ReactDOM from 'react-dom'
1414
import Head from '../shared/lib/head'
@@ -44,7 +44,7 @@ export type { ImageLoaderProps }
4444
export type ImageLoader = (p: ImageLoaderProps) => string
4545

4646
type ImgElementWithDataProp = HTMLImageElement & {
47-
'data-loaded-src': string | undefined
47+
'data-loaded-src'?: string | undefined
4848
}
4949

5050
type ImageElementProps = ImgProps & {
@@ -180,6 +180,15 @@ function getDynamicProps(
180180
return { fetchpriority: fetchPriority }
181181
}
182182

183+
/**
184+
* A version of useLayoutEffect that doesn't warn during SSR.
185+
* TODO: Just useLayoutEffect once support for React 18 is dropped.
186+
* Do not rename this to "isomorphic layout effect". There is no such thing as
187+
* an isomorphic Layout Effect since there is no Layout on the server
188+
*/
189+
const useNonWarningLayoutEffect =
190+
typeof window === 'undefined' ? useEffect : useLayoutEffect
191+
183192
const ImageElement = forwardRef<HTMLImageElement | null, ImageElementProps>(
184193
(
185194
{
@@ -207,18 +216,25 @@ const ImageElement = forwardRef<HTMLImageElement | null, ImageElementProps>(
207216
},
208217
forwardedRef
209218
) => {
210-
const ownRef = useCallback(
211-
(img: ImgElementWithDataProp | null) => {
212-
if (!img) {
213-
return
214-
}
219+
const didInsertRef = useRef(false)
220+
const insertedImgRef = useRef<HTMLImageElement>(null)
221+
222+
useNonWarningLayoutEffect(() => {
223+
const { current: didInsert } = didInsertRef
224+
const { current: img } = insertedImgRef
225+
226+
if (!didInsert && img !== null) {
227+
// Replay events from during hydration that React doesn't replay.
215228
if (onError) {
216229
// If the image has an error before react hydrates, then the error is lost.
217230
// The workaround is to wait until the image is mounted which is after hydration,
218231
// then we set the src again to trigger the error handler (if there was an error).
232+
// This doesn't just trigger the error handler but retries the whole request.
233+
// TODO: Consider dispatching a synthetic event instead.
219234
// eslint-disable-next-line no-self-assign
220235
img.src = img.src
221236
}
237+
222238
if (process.env.NODE_ENV !== 'production') {
223239
if (!src) {
224240
console.error(`Image is missing required "src" property:`, img)
@@ -240,22 +256,24 @@ const ImageElement = forwardRef<HTMLImageElement | null, ImageElementProps>(
240256
sizesInput
241257
)
242258
}
243-
},
244-
[
245-
src,
246-
placeholder,
247-
onLoadRef,
248-
onLoadingCompleteRef,
249-
setBlurComplete,
250-
onError,
251-
unoptimized,
252-
sizesInput,
253-
]
254-
)
259+
didInsertRef.current = true
260+
}
261+
}, [
262+
src,
263+
placeholder,
264+
onLoadRef,
265+
onLoadingCompleteRef,
266+
onError,
267+
unoptimized,
268+
sizesInput,
269+
])
255270

256-
const ref = useMergedRef(forwardedRef, ownRef)
271+
const ref = useMergedRef(forwardedRef, insertedImgRef)
257272

258273
return (
274+
// If you move this element creation, also move the Layout Effect above
275+
// reading from the ref. Otherwise we might run the Layout Effect when
276+
// the current value isn't set to the HTMLImageElement instance.
259277
<img
260278
{...rest}
261279
{...getDynamicProps(fetchPriority)}
@@ -280,9 +298,9 @@ const ImageElement = forwardRef<HTMLImageElement | null, ImageElementProps>(
280298
src={src}
281299
ref={ref}
282300
onLoad={(event) => {
283-
const img = event.currentTarget as ImgElementWithDataProp
301+
const currentImage = event.currentTarget
284302
handleLoading(
285-
img,
303+
currentImage,
286304
placeholder,
287305
onLoadRef,
288306
onLoadingCompleteRef,

‎test/e2e/app-dir/next-image-events/app/fulfilled/page.tsx‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
'use client'
2+
'use no memo'
3+
4+
import { useReducer, useState } from 'react'
5+
import Image from 'next/image'
6+
7+
export default function Page() {
8+
const [, setLoadEvent] = useState<unknown>(null)
9+
const [showClientImage, setShowClientImage] = useState(false)
10+
const [, rerender] = useReducer((i) => i + 1, 0)
11+
12+
return (
13+
<>
14+
<Image
15+
alt="foo"
16+
width={5}
17+
height={5}
18+
unoptimized
19+
src="/test.png?hydrated"
20+
onError={() => {}}
21+
onLoad={(event) => {
22+
console.error('hydrated image load')
23+
// This doesn't really make sense. We just want to check rerendering
24+
// doesn't infinitely loop
25+
setLoadEvent(event)
26+
}}
27+
/>
28+
<button type="button" onClick={rerender}>
29+
rerender Page
30+
</button>
31+
<button type="button" onClick={() => setShowClientImage(true)}>
32+
Show Client image
33+
</button>
34+
{showClientImage && (
35+
<Image
36+
alt="bar"
37+
width={5}
38+
height={5}
39+
unoptimized
40+
src="/test.png?client"
41+
onError={() => {}}
42+
onLoad={(event) => {
43+
console.error('client rendered image load')
44+
// This doesn't really make sense. We just want to check rerendering
45+
// doesn't infinitely loop
46+
setLoadEvent(event)
47+
}}
48+
/>
49+
)}
50+
</>
51+
)
52+
}

‎test/e2e/app-dir/next-image-events/app/layout.tsx‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import { ReactNode } from 'react'
2+
export default function Root({ children }: { children: ReactNode }) {
3+
return (
4+
<html>
5+
<body>{children}</body>
6+
</html>
7+
)
8+
}

‎test/e2e/app-dir/next-image-events/app/rejected/page.tsx‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
'use client'
2+
'use no memo'
3+
4+
import { useReducer, useState } from 'react'
5+
import Image from 'next/image'
6+
7+
export default function Page() {
8+
const [, setErrorEvent] = useState<unknown>(null)
9+
const [showClientImage, setShowClientImage] = useState(false)
10+
const [, rerender] = useReducer((i) => i + 1, 0)
11+
12+
return (
13+
<>
14+
<Image
15+
alt="foo"
16+
width={5}
17+
height={5}
18+
unoptimized
19+
src="/will-never-exist.png"
20+
onError={(event) => {
21+
console.error('hydrated image error')
22+
// This doesn't really make sense. We just want to check rerendering
23+
// doesn't infinitely loop
24+
setErrorEvent(event)
25+
}}
26+
/>
27+
<button type="button" onClick={rerender}>
28+
rerender Page
29+
</button>
30+
<button type="button" onClick={() => setShowClientImage(true)}>
31+
Show Client image
32+
</button>
33+
{showClientImage && (
34+
<Image
35+
alt="bar"
36+
width={5}
37+
height={5}
38+
unoptimized
39+
src="/still-doesnt-exist.png"
40+
onError={(event) => {
41+
console.error('client rendered image error')
42+
// This doesn't really make sense. We just want to check rerendering
43+
// doesn't infinitely loop
44+
setErrorEvent(event)
45+
}}
46+
/>
47+
)}
48+
</>
49+
)
50+
}

‎test/e2e/app-dir/next-image-events/next-image-events.test.ts‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
import { nextTestSetup } from 'e2e-utils'
2+
import { retry } from 'next-test-utils'
3+
4+
describe('next-image-events', () => {
5+
const { next } = nextTestSetup({
6+
files: __dirname,
7+
})
8+
9+
it('should not call onLoad multiple times', async () => {
10+
const imageRequests = []
11+
const browser = await next.browser('/fulfilled', {
12+
beforePageLoad(page) {
13+
page.on('request', (request) => {
14+
if (request.resourceType() === 'image') {
15+
imageRequests.push(request.url())
16+
}
17+
})
18+
},
19+
})
20+
21+
let logsIdx = 0
22+
await retry(async () => {
23+
const logs = await browser.log()
24+
expect(
25+
logs.slice(logsIdx).filter(({ source }) => source === 'error')
26+
).toEqual([
27+
{
28+
source: 'error',
29+
message: 'hydrated image load',
30+
},
31+
])
32+
logsIdx = logs.length
33+
})
34+
expect(imageRequests).toEqual([expect.stringContaining('test')])
35+
imageRequests.length = 0
36+
37+
await browser.locator(':text("Show Client image")').click()
38+
39+
await retry(async () => {
40+
const logs = await browser.log()
41+
expect(
42+
logs.slice(logsIdx).filter(({ source }) => source === 'error')
43+
).toEqual([
44+
{
45+
source: 'error',
46+
message: 'client rendered image load',
47+
},
48+
])
49+
logsIdx = logs.length
50+
})
51+
expect(imageRequests).toEqual([expect.stringContaining('test')])
52+
imageRequests.length = 0
53+
54+
await browser.locator(':text("rerender Page")').click()
55+
56+
const logs = await browser.log()
57+
expect(
58+
logs.slice(logsIdx).filter(({ source }) => source === 'error')
59+
).toEqual([])
60+
expect(imageRequests).toEqual([])
61+
imageRequests.length = 0
62+
})
63+
64+
it('should not infinitely retry on error', async () => {
65+
const nextImageRequests: string[] = []
66+
let logsIdx = 0
67+
const browser = await next.browser('/rejected', {
68+
beforePageLoad(page) {
69+
// We're manually aborting here to simulate an error before React hydrates.
70+
// A real request might settle too late to test this behavior reliably.
71+
// Especially in dev, requests (even if warm) may take longer than hydration.
72+
page.route(
73+
/\/will-never-exist\.png|\/still-doesnt-exist\.png/,
74+
(route) => {
75+
nextImageRequests.push(route.request().url())
76+
return route.abort()
77+
}
78+
)
79+
},
80+
})
81+
82+
await retry(async () => {
83+
const logs = await browser.log()
84+
expect(
85+
logs.slice(logsIdx).filter(({ source }) => source === 'error')
86+
).toEqual([
87+
{
88+
source: 'error',
89+
message: 'Failed to load resource: net::ERR_FAILED',
90+
},
91+
// Next.js retries once to trigger onError on SSRed, settled images.
92+
// If this test fails, we'll either hydrated faster than the request settled,
93+
// or dropped the retrying behavior of next/image
94+
{
95+
source: 'error',
96+
message: 'Failed to load resource: net::ERR_FAILED',
97+
},
98+
{
99+
source: 'error',
100+
message: 'hydrated image error',
101+
},
102+
])
103+
logsIdx = logs.length
104+
})
105+
expect(nextImageRequests).toEqual([
106+
expect.stringContaining('will-never-exist'),
107+
// Next.js retries once to trigger onError on SSRed, settled images.
108+
// If this test fails, we'll either hydrated faster than the request settled,
109+
// or dropped the retrying behavior of next/image
110+
expect.stringContaining('will-never-exist'),
111+
])
112+
nextImageRequests.length = 0
113+
114+
await browser.locator(':text("Show Client image")').click()
115+
116+
await retry(async () => {
117+
const logs = await browser.log()
118+
expect(
119+
logs.slice(logsIdx).filter(({ source }) => source === 'error')
120+
).toEqual([
121+
{
122+
source: 'error',
123+
message: 'Failed to load resource: net::ERR_FAILED',
124+
},
125+
{
126+
source: 'error',
127+
message: 'client rendered image error',
128+
},
129+
])
130+
logsIdx = logs.length
131+
})
132+
expect(nextImageRequests).toEqual([
133+
expect.stringContaining('still-doesnt-exist'),
134+
])
135+
nextImageRequests.length = 0
136+
137+
await browser.locator(':text("rerender Page")').click()
138+
139+
const logs = await browser.log()
140+
expect(
141+
logs.slice(logsIdx).filter(({ source }) => source === 'error')
142+
).toEqual([])
143+
expect(nextImageRequests).toEqual([])
144+
nextImageRequests.length = 0
145+
logsIdx = logs.length
146+
})
147+
})

‎test/e2e/app-dir/next-image-events/next.config.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
/**
2+
* @type {import('next').NextConfig}
3+
*/
4+
const nextConfig = {}
5+
6+
module.exports = nextConfig

‎test/e2e/app-dir/next-image-events/public/test.png‎

1.51 KB
Loading

0 commit comments

Comments
 (0)