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.

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 onError handling in next/future/image #39824

Merged
merged 4 commits into from Aug 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 1 addition & 4 deletions docs/api-reference/next/future/image.md
Expand Up @@ -43,7 +43,6 @@ Compared to `next/image`, the new `next/future/image` component has the followin
- Removes `lazyBoundary` prop since there is no native equivalent
- Removes `lazyRoot` prop since there is no native equivalent
- Removes `loader` config in favor of [`loader`](#loader) prop
- Note: the [`onError`](#onerror) prop might behave differently

## Known Browser Bugs

Expand Down Expand Up @@ -316,16 +315,14 @@ The callback function will be called with one argument, an object with the follo

A callback function that is invoked when the image is loaded.

Note that the load event might occur before client-side hydration completes, so this callback might not be invoked in that case.
Note that the load event might occur before the placeholder is removed and the image is fully decoded.

Instead, use [`onLoadingComplete`](#onloadingcomplete).

### onError

A callback function that is invoked if the image fails to load.

Note that the error might occur before client-side hydration completes, so this callback might not be invoked in that case.

### loading

> **Attention**: This property is only meant for advanced usage. Switching an
Expand Down
17 changes: 15 additions & 2 deletions packages/next/client/future/image.tsx
Expand Up @@ -358,7 +358,14 @@ const ImageElement = ({
loading={loading}
style={{ ...imgStyle, ...blurStyle }}
ref={useCallback(
(img: ImgElementWithDataProp) => {
(img: ImgElementWithDataProp | null) => {
if (img && onError) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this conditioned on onError only? can't a custom onLoad be provided too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was specifically concerned about onError because we already have a workaround for onLoad which is onLoadingComplete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, i glanced at it, seems like it is doing quite a bit of stuff. If it were simply dealing with the load stuff it would be nice to unify with the error path here. Is all that complexity intrinsically necessary or is it b/c working around onLoad for hydration was a pain?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onLoadingComplete does a few things:

  • ensures the image is fully decoded
  • removes blur placeholder (not always necessary when svg background used but still necessary for css filter)
  • guards against the react hydration issue since it runs even when onLoad was never called

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it run onLoad though? if the user provides a custom onLoad and onError to every img but only the onErrors get called for errors the symmetry is still off?

Copy link
Member Author

@styfle styfle Aug 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No the user must provide onLoadingComplete.

We have always discouraged onLoad usage in both next/image and next/future/image for the 3 reasons above.

If we can solve all the corner cases for svg placeholder, we can probably start supporting onLoad in the future.

// If the image has an error before react hydrates, then the error is lost.
// The workaround is to wait until the image is mounted which is after hydration,
// then we set the src again to trigger the error handler (if there was an error).
// eslint-disable-next-line no-self-assign
img.src = img.src
}
if (process.env.NODE_ENV !== 'production') {
if (img && !srcString) {
console.error(`Image is missing required "src" property:`, img)
Expand All @@ -374,7 +381,13 @@ const ImageElement = ({
)
}
},
[srcString, placeholder, onLoadingCompleteRef, setBlurComplete]
[
srcString,
placeholder,
onLoadingCompleteRef,
setBlurComplete,
onError,
]
)}
onLoad={(event) => {
const img = event.currentTarget as ImgElementWithDataProp
Expand Down
@@ -0,0 +1,22 @@
import { useState } from 'react'
import Image from 'next/future/image'

const Page = () => {
const [msg, setMsg] = useState(`default state`)
return (
<div>
<Image
id="img"
src="/404.jpg"
width="200"
height="200"
onError={() => {
setMsg(`error state`)
}}
/>
<p id="msg">{msg}</p>
</div>
)
}

export default Page
8 changes: 8 additions & 0 deletions test/integration/image-future/default/test/index.test.ts
Expand Up @@ -387,6 +387,14 @@ function runTests(mode) {
)
})

it('should callback native onError even when error before hydration', async () => {
let browser = await webdriver(appPort, '/on-error-before-hydration')
await check(
() => browser.eval(`document.getElementById("msg").textContent`),
'error state'
)
})

Comment on lines +390 to +397
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't really assert anything about which branch of the race condition is being tested. Was a similar test failing 100% of the time before?

Can you make it so next's hydration waits until after the img has errored explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test title says “before hydration” and the page name has before-hydration.

I also confirmed this reproduces every time locally.

I don’t think we can write a e2e test that delays Next hydration during “next start”

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could potentially add a test method similar to the __NEXT_HYDRATED_CB we use to delay hydrating until a Promise is resolved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test title says “before hydration” and the page name has before-hydration.

Right I meant that the test suggests there is an error before hydration but it does not guarantee the error happened before hydration so in the future if we deleted this fix accidentally the test could very well continue to pass even though it should start failing.

I also confirmed this reproduces every time locally.

So if this is the case I suppose it's ok but that could potentially change in the future depending on where the tests are run or how Next or React changes. It get's a little heavy handed I suppose to try to contrive these things but the test would feel a lot more valuable if you knew it was fail 100% of the time if the fix was removed.

Copy link
Member Author

@styfle styfle Aug 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if react could fetch, parse, and hydrate before the <img> is fetched and errors, its possible the new code path would not be executed and thus the new test fixture would be useless. I'll see if we can delay hydration, its just something we can do with a production build

it('should work with image with blob src', async () => {
let browser
try {
Expand Down