fix: support onLoadingComplete in modern next/image shim#496
Conversation
Add the deprecated-but-still-supported onLoadingComplete prop to the modern next/image component, matching Next.js behavior where the callback receives the underlying HTMLImageElement on load. - Add onLoadingComplete to ImageProps interface and type declarations - Wire handleLoad into all render paths (local, custom loader, remote) - Destructure onLoad/onLoadingComplete in getImageProps to prevent leak
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: onLoadingComplete support in modern next/image shim
Good feature addition — the implementation covers all render paths (local, custom loader, remote/UnpicImage) and correctly destructures the prop in getImageProps to prevent leaking. The tests are well-targeted. Two issues to address:
1. Callback invocation order is reversed vs Next.js (bug)
In Next.js's modern next/image, when both onLoad and onLoadingComplete are provided, onLoad fires first, then onLoadingComplete. This PR fires them in the opposite order. This is a behavioral difference that could break code relying on side effects in onLoad completing before onLoadingComplete runs.
2. Stale comment describes legacy behavior, not modern
The comment says "Calls the callback with { naturalWidth, naturalHeight }" but the code actually passes HTMLImageElement (which is correct for modern next/image). The comment describes the legacy image behavior.
Test coverage
The tests are solid for prop leak prevention and SSR attribute sanitization. One gap: there's no test for the remote image (UnpicImage) SSR path — the tests only cover local images and custom loaders. A remote URL test would complete the coverage for all three render paths.
Overall this is a clean, well-scoped change. Fix the invocation order and the comment, and it's good to go.
| const handleLoad = onLoadingComplete | ||
| ? (e: React.SyntheticEvent<HTMLImageElement>) => { | ||
| onLoadingComplete(e.currentTarget); | ||
| onLoad?.(e); |
There was a problem hiding this comment.
Bug: invocation order reversed vs Next.js. In the upstream modern next/image (image-component.tsx), when both callbacks are present, onLoad fires before onLoadingComplete. This PR fires onLoadingComplete first.
This matters because user code may rely on onLoad side effects completing before onLoadingComplete runs.
| const handleLoad = onLoadingComplete | |
| ? (e: React.SyntheticEvent<HTMLImageElement>) => { | |
| onLoadingComplete(e.currentTarget); | |
| onLoad?.(e); | |
| const handleLoad = onLoadingComplete | |
| ? (e: React.SyntheticEvent<HTMLImageElement>) => { | |
| onLoad?.(e); | |
| onLoadingComplete(e.currentTarget); | |
| } | |
| : onLoad; |
Note: the legacy image shim correctly fires onLoadingComplete first (matching legacy Next.js behavior), so only the modern shim needs this fix.
| // Wire onLoadingComplete (deprecated) into onLoad — matches Next.js behavior. | ||
| // Calls the callback with { naturalWidth, naturalHeight } after the image loads. |
There was a problem hiding this comment.
The comment says "Calls the callback with { naturalWidth, naturalHeight }" but the code passes HTMLImageElement (line 215). The { naturalWidth, naturalHeight } form is the legacy next/legacy/image signature. The modern next/image signature (which this shim implements) passes the raw HTMLImageElement.
| // Wire onLoadingComplete (deprecated) into onLoad — matches Next.js behavior. | |
| // Calls the callback with { naturalWidth, naturalHeight } after the image loads. | |
| // Wire onLoadingComplete (deprecated) into onLoad — matches Next.js behavior. | |
| // Calls the callback with the HTMLImageElement after the image loads. |
|
Review posted successfully. Here's a summary of the feedback: Two issues found:
I also noted that test coverage could be improved with a remote URL (UnpicImage) SSR test to complete coverage across all three render paths, though the existing tests for prop leak prevention and local/custom-loader SSR are solid. |
- Swap onLoad/onLoadingComplete invocation order to match Next.js
(onLoad fires first, then onLoadingComplete)
- Fix stale comment that described legacy { naturalWidth, naturalHeight }
- Add remote URL (UnpicImage) SSR test for full path coverage
|
/bigbonk re-review |
There was a problem hiding this comment.
Clean, well-scoped change. The implementation correctly handles all three render paths (local, custom loader, remote/UnpicImage), the callback order matches Next.js (onLoad before onLoadingComplete), and both props are properly stripped in getImageProps.
The second commit addressed the earlier review feedback (callback order swap, stale comment fix, remote URL test). All 34 image component tests pass.
One minor note below, but nothing blocking.
| className, | ||
| style, | ||
| onLoad: _onLoad, | ||
| onLoadingComplete: _onLoadingComplete, |
There was a problem hiding this comment.
Nit: getImageProps previously passed onLoad through to the returned props via ...rest (since it wasn't destructured). This PR now strips it by destructuring as _onLoad. That's actually the correct behavior — Next.js's getImageProps also strips event handlers from the output — but it's worth noting this is a subtle secondary fix bundled into the PR.
|
Review posted. Here's a summary of my findings: Approved — this is a clean, well-scoped change. What the PR does:
What I verified:
|
2 participants
Summary
onLoadingCompleteprop to the modernnext/imageshimHTMLImageElement(matching Next.js modern image behavior, not the{ naturalWidth, naturalHeight }form from legacy image)getImagePropsdestructures bothonLoadandonLoadingCompleteso neither leaks into returned propsCloses #15
Test plan
tests/image-component.test.tscovering prop leak prevention viagetImagePropsand SSR output for local + custom loader paths