Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/vinext/src/shims/image.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ interface ImageProps {
className?: string;
style?: React.CSSProperties;
onLoad?: React.ReactEventHandler<HTMLImageElement>;
/** @deprecated Use onLoad instead. Still supported for migration compat. */
onLoadingComplete?: (img: HTMLImageElement) => void;
onError?: React.ReactEventHandler<HTMLImageElement>;
onClick?: React.MouseEventHandler<HTMLImageElement>;
id?: string;
Expand Down Expand Up @@ -197,13 +199,24 @@ const Image = forwardRef<HTMLImageElement, ImageProps>(function Image(
sizes,
className,
style,
onLoad,
onLoadingComplete,
unoptimized: _unoptimized,
overrideSrc: _overrideSrc,
loading,
...rest
},
ref,
) {
// Wire onLoadingComplete (deprecated) into onLoad — matches Next.js behavior.
// onLoad fires first, then onLoadingComplete receives the HTMLImageElement.
const handleLoad = onLoadingComplete
? (e: React.SyntheticEvent<HTMLImageElement>) => {
onLoad?.(e);
Comment on lines +213 to +215
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
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.

onLoadingComplete(e.currentTarget);
}
: onLoad;

// Handle StaticImageData (import result)
const src = typeof srcProp === "string" ? srcProp : srcProp.src;
const imgWidth = width ?? (typeof srcProp === "object" ? srcProp.width : undefined);
Expand All @@ -225,6 +238,7 @@ const Image = forwardRef<HTMLImageElement, ImageProps>(function Image(
decoding="async"
sizes={sizes}
className={className}
onLoad={handleLoad}
style={
fill
? {
Expand Down Expand Up @@ -269,6 +283,7 @@ const Image = forwardRef<HTMLImageElement, ImageProps>(function Image(
sizes={sizes}
className={className}
background={bg}
onLoad={handleLoad}
/>
);
}
Expand All @@ -285,6 +300,7 @@ const Image = forwardRef<HTMLImageElement, ImageProps>(function Image(
sizes={sizes}
className={className}
background={bg}
onLoad={handleLoad}
/>
);
}
Expand Down Expand Up @@ -350,6 +366,7 @@ const Image = forwardRef<HTMLImageElement, ImageProps>(function Image(
sizes={sizes ?? (fill ? "100vw" : undefined)}
className={className}
data-nimg={fill ? "fill" : "1"}
onLoad={handleLoad}
style={
fill
? {
Expand Down Expand Up @@ -389,6 +406,8 @@ export function getImageProps(props: ImageProps): {
sizes,
className,
style,
onLoad: _onLoad,
onLoadingComplete: _onLoadingComplete,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

unoptimized: _unoptimized,
overrideSrc: _overrideSrc,
loading,
Expand Down
2 changes: 2 additions & 0 deletions packages/vinext/src/shims/next-shims.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ declare module "next/image" {
className?: string;
style?: CSSProperties;
onLoad?: ReactEventHandler<HTMLImageElement>;
/** @deprecated Use onLoad instead. Still supported for migration compat. */
onLoadingComplete?: (img: HTMLImageElement) => void;
onError?: ReactEventHandler<HTMLImageElement>;
onClick?: MouseEventHandler<HTMLImageElement>;
id?: string;
Expand Down
78 changes: 78 additions & 0 deletions tests/image-component.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -477,3 +477,81 @@ describe("blurDataURL CSS injection prevention", () => {
expect(html).toContain(validURL);
});
});

// ─── onLoadingComplete (deprecated but supported) ────────────────────────
// Next.js deprecated onLoadingComplete in v14 but still supports it.
// It should be handled internally and NOT leak through to the returned props.

describe("onLoadingComplete prop", () => {
it("getImageProps does not leak onLoadingComplete into returned props", () => {
const { props } = getImageProps({
alt: "test",
src: "/photo.jpg",
width: 400,
height: 300,
onLoadingComplete: () => {},
});
// onLoadingComplete must be consumed internally, not passed through
expect((props as any).onLoadingComplete).toBeUndefined();
});

it("getImageProps does not leak onLoad or onLoadingComplete when both provided", () => {
const { props } = getImageProps({
alt: "test",
src: "/photo.jpg",
width: 400,
height: 300,
onLoad: () => {},
onLoadingComplete: () => {},
});
expect((props as any).onLoadingComplete).toBeUndefined();
expect((props as any).onLoad).toBeUndefined();
});

it("does not leak onLoadingComplete as a DOM attribute in SSR (local image)", () => {
const html = ReactDOMServer.renderToString(
React.createElement(Image, {
alt: "test",
src: "/photo.jpg",
width: 400,
height: 300,
onLoadingComplete: () => {},
}),
);
expect(html).not.toContain("onLoadingComplete");
expect(html).not.toContain("onloadingcomplete");
expect(html).toContain('alt="test"');
});

it("does not leak onLoadingComplete as a DOM attribute in SSR (custom loader)", () => {
const html = ReactDOMServer.renderToString(
React.createElement(Image, {
alt: "cdn",
src: "/photo.jpg",
width: 200,
height: 150,
loader: ({ src, width }: { src: string; width: number }) =>
`https://cdn.example.com${src}?w=${width}`,
onLoadingComplete: () => {},
}),
);
expect(html).not.toContain("onLoadingComplete");
expect(html).not.toContain("onloadingcomplete");
expect(html).toContain('alt="cdn"');
});

it("does not leak onLoadingComplete as a DOM attribute in SSR (remote URL)", () => {
const html = ReactDOMServer.renderToString(
React.createElement(Image, {
alt: "remote",
src: "https://example.com/photo.jpg",
width: 400,
height: 300,
onLoadingComplete: () => {},
}),
);
expect(html).not.toContain("onLoadingComplete");
expect(html).not.toContain("onloadingcomplete");
expect(html).toContain('alt="remote"');
});
});
Loading