-
Notifications
You must be signed in to change notification settings - Fork 326
fix(image): emit preload hints for priority images #1266
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
base: main
Are you sure you want to change the base?
Changes from all 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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -44,7 +44,7 @@ describe("Image SSR rendering", () => { | |||||||
| expect(html).toContain('data-nimg="1"'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it("renders with priority (eager loading + fetchpriority)", () => { | ||||||||
| it("renders with priority (preload + eager loading + fetchpriority)", () => { | ||||||||
| const html = ReactDOMServer.renderToString( | ||||||||
| React.createElement(Image, { | ||||||||
| alt: "priority image", | ||||||||
|
|
@@ -54,11 +54,36 @@ describe("Image SSR rendering", () => { | |||||||
| priority: true, | ||||||||
| }), | ||||||||
| ); | ||||||||
| // Ported from Next.js: | ||||||||
| // .nextjs-ref/test/e2e/next-image-new/app-dir/app-dir-static.test.ts | ||||||||
| // .nextjs-ref/packages/next/src/client/image-component.tsx | ||||||||
| expect(html).toContain('<link rel="preload"'); | ||||||||
| expect(html).toContain('as="image"'); | ||||||||
| expect(html).toContain('fetchPriority="high"'); | ||||||||
| expect(html).toContain(`imageSrcSet="${optUrlHtml("/hero.png", 640)} 640w`); | ||||||||
| expect(html).not.toContain(`href="${optUrlHtml("/hero.png", 800)}"`); | ||||||||
| expect(html).toContain('loading="eager"'); | ||||||||
| expect(html).toContain('fetchPriority="high"'); | ||||||||
| expect(html).not.toContain('loading="lazy"'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it("renders an image preload for the modern preload prop", () => { | ||||||||
| const html = ReactDOMServer.renderToString( | ||||||||
| React.createElement(Image, { | ||||||||
| alt: "preloaded image", | ||||||||
| src: "/hero-preload.png", | ||||||||
| width: 800, | ||||||||
| height: 600, | ||||||||
| preload: true, | ||||||||
| }), | ||||||||
| ); | ||||||||
| expect(html).toContain('<link rel="preload"'); | ||||||||
| expect(html).toContain('as="image"'); | ||||||||
| expect(html).toContain(`imageSrcSet="${optUrlHtml("/hero-preload.png", 640)} 640w`); | ||||||||
| expect(html).not.toContain('loading="lazy"'); | ||||||||
|
Contributor
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. Nit: This assertion confirms
Suggested change
|
||||||||
| expect(html).not.toContain('fetchPriority="high"'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it("renders fill mode with absolute positioning", () => { | ||||||||
| const html = ReactDOMServer.renderToString( | ||||||||
| React.createElement(Image, { | ||||||||
|
|
||||||||
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.
Nit: The
typeof ReactDOM.preload !== 'function'guard is good, but in Next.js theImagePreloadcomponent also passescrossOriginandreferrerPolicyto the preload options (since those affect CORS and the browser's preload matching). vinext'sImagecomponent doesn't accept these props today, so nothing to fix here, but worth noting as a follow-up gap — ifcrossOriginorreferrerPolicysupport is added to the Image component later, the preload call should be updated to pass them too.