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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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” There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could potentially add a test method similar to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah if react could fetch, parse, and hydrate before the |
||
it('should work with image with blob src', async () => { | ||
let browser | ||
try { | ||
|
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.
Why is this conditioned on
onError
only? can't a custom onLoad be provided too?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.
I was specifically concerned about onError because we already have a workaround for onLoad which is onLoadingComplete.
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.
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?
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.
onLoadingComplete
does a few things:onLoad
was never calledThere 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.
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?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.
No the user must provide
onLoadingComplete
.We have always discouraged
onLoad
usage in bothnext/image
andnext/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.