Fix `next/image` usage of `onError()` #36305

shinkj11 · 2022-04-20T10:36:46Z

History

In PR #35889, onError prop was added explicitly on the Image component, but it was ommitted from the ImageElement component. In result, the callback didn't work at all.

Now

This PR deletes the onError prop from explicitly defined props. Explicitly adding onError on the const imgElementArgs is considered, but deleting the defined prop from Image is more reasonable because the prop isn't used in Image component.

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

styfle

Thanks for the PR!

Looks like this is similar to the onLoad() bug that was fixed in #36176

Let's add a similar test here for onError() to make sure its working properly.

…1/next-js-official into bugfix/image-onerror-fix

shinkj11 · 2022-04-22T09:17:08Z

Tests are added. I can't find the way to fire error event except for using invalid src path. If there's any other error firing situation, I'll added to the cases.

ijjk · 2022-04-22T11:46:25Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
buildDuration	15.6s	15.7s	⚠️ +92ms
buildDurationCached	6.1s	6.1s	-82ms
nodeModulesSize	481 MB	481 MB	-67 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.17	3.197	⚠️ +0.03
/ avg req/sec	788.54	782.06	⚠️ -6.48
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.362	1.358	0
/error-in-render avg req/sec	1836.03	1840.47	+4.44

Client Bundles (main, webpack)

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	28.6 kB	28.6 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	72.3 kB	72.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages Overall decrease ✓

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	192 B	192 B	✓
amp-HASH.js gzip	309 B	309 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	3.04 kB	3.04 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.74 kB	5.73 kB	-8 B
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.36 kB	2.36 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	392 B	392 B	✓
withRouter-HASH.js gzip	319 B	319 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	16 kB	16 kB	-8 B

Client Build Manifests

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
_buildManifest.js gzip	461 B	461 B	✓
Overall change	461 B	461 B	✓

Rendered Page Sizes

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
index.html gzip	531 B	531 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	525 B	525 B	✓
Overall change	1.6 kB	1.6 kB	✓

Diffs

Diff for _buildManifest.js

@@ -12,7 +12,7 @@ self.__BUILD_MANIFEST = {
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-96a5d6ed07cf5a83.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-9dfe734f583d4926.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-9a0a6e890d6a266a.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-4ebd299a1253d245.js"],
   "/link": ["static\u002Fchunks\u002Fpages\u002Flink-c605640c895e01ab.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-98eb70bf22fb21da.js"

Diff for image-HASH.js

@@ -149,7 +149,6 @@
           objectFit = _param.objectFit,
           objectPosition = _param.objectPosition,
           onLoadingComplete = _param.onLoadingComplete,
-          onError = _param.onError,
           _placeholder = _param.placeholder,
           placeholder = _placeholder === void 0 ? "empty" : _placeholder,
           blurDataURL = _param.blurDataURL,
@@ -169,7 +168,6 @@
             "objectFit",
             "objectPosition",
             "onLoadingComplete",
-            "onError",
             "placeholder",
             "blurDataURL"
           ]);

Default Build with SWC (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
buildDuration	17.6s	18s	⚠️ +433ms
buildDurationCached	6.1s	6.2s	⚠️ +27ms
nodeModulesSize	481 MB	481 MB	-67 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.241	3.188	-0.05
/ avg req/sec	771.45	784.2	+12.75
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.392	1.399	⚠️ +0.01
/error-in-render avg req/sec	1796.03	1786.81	⚠️ -9.22

Client Bundles (main, webpack)

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	29.1 kB	29.1 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.9 kB	72.9 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages Overall decrease ✓

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	179 B	179 B	✓
amp-HASH.js gzip	313 B	313 B	✓
css-HASH.js gzip	325 B	325 B	✓
dynamic-HASH.js gzip	3.02 kB	3.02 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	921 B	921 B	✓
image-HASH.js gzip	5.79 kB	5.78 kB	-9 B
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.44 kB	2.44 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
script-HASH.js gzip	393 B	393 B	✓
withRouter-HASH.js gzip	317 B	317 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	16.1 kB	16.1 kB	-9 B

Client Build Manifests

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
_buildManifest.js gzip	457 B	457 B	✓
Overall change	457 B	457 B	✓

Rendered Page Sizes

	vercel/next.js canary	shinkj11/next-js-official bugfix/image-onerror-fix	Change
index.html gzip	531 B	531 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	526 B	526 B	✓
Overall change	1.6 kB	1.6 kB	✓

Diffs

Diff for _buildManifest.js

@@ -12,7 +12,7 @@ self.__BUILD_MANIFEST = {
   ],
   "/head": ["static\u002Fchunks\u002Fpages\u002Fhead-96a5d6ed07cf5a83.js"],
   "/hooks": ["static\u002Fchunks\u002Fpages\u002Fhooks-9dfe734f583d4926.js"],
-  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-9a0a6e890d6a266a.js"],
+  "/image": ["static\u002Fchunks\u002Fpages\u002Fimage-4ebd299a1253d245.js"],
   "/link": ["static\u002Fchunks\u002Fpages\u002Flink-c605640c895e01ab.js"],
   "/routerDirect": [
     "static\u002Fchunks\u002Fpages\u002FrouterDirect-98eb70bf22fb21da.js"

Diff for image-HASH.js

@@ -149,7 +149,6 @@
           objectFit = _param.objectFit,
           objectPosition = _param.objectPosition,
           onLoadingComplete = _param.onLoadingComplete,
-          onError = _param.onError,
           _placeholder = _param.placeholder,
           placeholder = _placeholder === void 0 ? "empty" : _placeholder,
           blurDataURL = _param.blurDataURL,
@@ -169,7 +168,6 @@
             "objectFit",
             "objectPosition",
             "onLoadingComplete",
-            "onError",
             "placeholder",
             "blurDataURL"
           ]);

Commit: 9d90d9f

test/integration/image-component/default/test/index.test.js

styfle

Perfect, thanks! 🏅

xiaoxiaosaohuo · 2022-04-23T07:26:10Z

thanks, this is a very import PR, hoping to release it soon

djejaquino · 2022-04-25T18:36:52Z

Good find @shinkj11

If anyone needs a quick fix for this, you might look into using onErrorCapture event instead.

styfle · 2022-04-26T13:31:20Z

The fix has been released to the canary channel. You can try it out today with npm install next@canary, thanks!

djejaquino · 2022-04-26T16:24:47Z

@styfle how long usually these sit in canary before going live in prod? Thanks

styfle · 2022-04-27T21:35:24Z

It depends if we find bugs on canary, but usually a couple weeks between stable patch releases.

Update image.tsx

2832837

shinkj11 requested review from timneutkens, ijjk, shuding and huozhi as code owners April 20, 2022 10:36

ijjk added the type: next label Apr 20, 2022

Merge branch 'canary' into bugfix/image-onerror-fix

4f5e926

shinkj11 changed the title ~~Update image.tsx~~ Fix onError omission of Image Apr 20, 2022

styfle reviewed Apr 21, 2022

View reviewed changes

kyeongjun.shin added 2 commits April 22, 2022 17:55

Add test for image onError

75d4676

Merge branch 'bugfix/image-onerror-fix' of https://github.com/shinkj1…

b084833

…1/next-js-official into bugfix/image-onerror-fix

styfle reviewed Apr 22, 2022

View reviewed changes

test/integration/image-component/default/test/index.test.js Outdated Show resolved Hide resolved

styfle changed the title ~~Fix onError omission of Image~~ Fix next/image usage of onError() Apr 22, 2022

Remove unused setTimeout

dfc6e09

styfle approved these changes Apr 22, 2022

View reviewed changes

ijjk approved these changes Apr 22, 2022

View reviewed changes

Merge branch 'canary' into bugfix/image-onerror-fix

9d90d9f

kodiakhq bot merged commit 42c01dd into vercel:canary Apr 22, 2022

balazsorban44 mentioned this pull request May 5, 2022

next/image 404 no events are fired #36694

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators May 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `next/image` usage of `onError()` #36305

Fix `next/image` usage of `onError()` #36305

shinkj11 commented Apr 20, 2022 •

edited by balazsorban44

styfle left a comment

shinkj11 commented Apr 22, 2022

ijjk commented Apr 22, 2022 •

edited

Diffs

Diffs

styfle left a comment

xiaoxiaosaohuo commented Apr 23, 2022

djejaquino commented Apr 25, 2022

styfle commented Apr 26, 2022

djejaquino commented Apr 26, 2022

styfle commented Apr 27, 2022

Fix next/image usage of onError() #36305

Fix next/image usage of onError() #36305

Conversation

shinkj11 commented Apr 20, 2022 • edited by balazsorban44

History

Now

Bug

styfle left a comment

Choose a reason for hiding this comment

shinkj11 commented Apr 22, 2022

ijjk commented Apr 22, 2022 • edited

Stats from current PR

Diffs

Diffs

styfle left a comment

Choose a reason for hiding this comment

xiaoxiaosaohuo commented Apr 23, 2022

djejaquino commented Apr 25, 2022

styfle commented Apr 26, 2022

djejaquino commented Apr 26, 2022

styfle commented Apr 27, 2022

Fix `next/image` usage of `onError()` #36305

Fix `next/image` usage of `onError()` #36305

shinkj11 commented Apr 20, 2022 •

edited by balazsorban44

ijjk commented Apr 22, 2022 •

edited