fix(image): strip priority prop before forwarding to UnpicImage to prevent DOM leak#662
fix(image): strip priority prop before forwarding to UnpicImage to prevent DOM leak#662james-elicx merged 2 commits intomainfrom
Conversation
…event DOM leak
The `priority` prop is a Next.js-specific concept that must never reach the
DOM as an attribute. On the two UnpicImage render paths (remote URL with fill
and remote URL with width+height), `priority={true}` was forwarded directly
to `@unpic/react`'s Image component which did not reliably strip it before
rendering the DOM `<img>`, triggering:
Received `true` for a non-boolean attribute `priority`.
Fix: replace `priority={priority}` with the equivalent HTML semantics —
`loading={priority ? 'eager' : loading ?? 'lazy'}` and
`fetchPriority={priority ? 'high' : undefined}` — matching what the local
image and custom-loader paths already do correctly.
Adds 10 reproduction tests covering both affected render paths.
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(image): strip priority prop before forwarding to UnpicImage
Clean, correct fix. The change is minimal and well-targeted.
Correctness
The priority → loading="eager" + fetchPriority="high" translation is the correct HTML semantic equivalent of Next.js's priority prop. This matches:
- How the same file already handles it for local images (lines 369-370)
- How the same file already handles it for custom loader path (line 237)
- How
getImageProps()already does it (lines 492-493)
All four code paths now handle priority consistently.
Completeness
Both affected <UnpicImage> call sites are fixed:
- Remote URL +
fill(fullWidth layout) — line 286-287 - Remote URL +
width+height(constrained layout) — line 305-306
The loading prop fallback chain priority ? "eager" : (loading ?? "lazy") correctly preserves an explicit loading prop when priority is not set, which matches the behavior of the other code paths.
Test coverage
The 6 new regression tests cover:
priority=not appearing in DOM output (both paths)loading="eager"present whenpriority=true(both paths)fetchPriority="high"present whenpriority=true- Default
loading="lazy"whenpriorityis unset
All 40 tests in image-component.test.ts pass.
Unrelated timing fix
The features.test.ts change relaxes a setTimeout(250ms) assertion from >= 250 to >= 200 — reasonable CI flakiness fix for JS timer imprecision. No behavioral impact.
LGTM.
|
Review posted. The PR is a clean, correct fix:
|
Problem
When using `next/image` with a remote URL and either `fill` or explicit `width`+`height`, the `priority` boolean prop was forwarded directly to `@unpic/react`'s `Image` component. Although `@unpic/core` is supposed to strip it via `transformSharedProps`, in practice it leaked through to the DOM `
` element, triggering:
```
Received `true` for a non-boolean attribute `priority`.
If you want to write it to the DOM, pass a string instead: priority="true" or priority={value.toString()}.
```
The two affected code paths were both in `packages/vinext/src/shims/image.tsx`:
Fix
Replace `priority={priority}` on both `` call sites with the equivalent HTML semantics that the local-image and custom-loader paths already use correctly:
```tsx
loading={priority ? "eager" : (loading ?? "lazy")}
fetchPriority={priority ? "high" : undefined}
```
`priority` is a Next.js-specific concept that has no equivalent HTML attribute. It must always be translated to `loading="eager"` + `fetchPriority="high"` before reaching the DOM.
Reproduction
Ten new tests added in `tests/image-component.test.ts` under `"priority prop — no DOM leak on remote URL paths"`: