From bb27851efad2ea002f055cbbd99347401a36ad5b Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 7 Jul 2023 20:04:13 -0400 Subject: [PATCH 1/7] fix: add missing `` for `next/image` in App Router --- packages/next/src/client/image-component.tsx | 78 +++++++++++++------ .../next-image-new/app-dir/test/index.test.ts | 46 +++++++---- .../next-image-new/default/test/index.test.ts | 48 +++++++----- 3 files changed, 114 insertions(+), 58 deletions(-) diff --git a/packages/next/src/client/image-component.tsx b/packages/next/src/client/image-component.tsx index 8e0777b045f78..33b18a4132d92 100644 --- a/packages/next/src/client/image-component.tsx +++ b/packages/next/src/client/image-component.tsx @@ -10,6 +10,7 @@ import React, { forwardRef, version, } from 'react' +import { preload } from 'react-dom' import Head from '../shared/lib/head' import { getImgProps } from '../shared/lib/get-img-props' import type { @@ -26,6 +27,7 @@ import type { import { imageConfigDefault } from '../shared/lib/image-config' import { ImageConfigContext } from '../shared/lib/image-config-context' import { warnOnce } from '../shared/lib/utils/warn-once' +import { RouterContext } from '../shared/lib/router-context' // @ts-ignore - This is replaced by webpack alias import defaultLoader from 'next/dist/shared/lib/image-loader' @@ -303,8 +305,57 @@ const ImageElement = forwardRef( } ) +function ImagePreload({ + isAppRouter, + imgAttributes, +}: { + isAppRouter: boolean + imgAttributes: ImgProps +}) { + const opts = { + as: 'image', + imageSrcSet: imgAttributes.srcSet, + imageSizes: imgAttributes.sizes, + crossOrigin: imgAttributes.crossOrigin, + referrerPolicy: imgAttributes.referrerPolicy, + ...getDynamicProps(imgAttributes.fetchPriority), + } + + if (isAppRouter) { + // See https://github.com/facebook/react/pull/26940 + // @ts-expect-error TODO: upgrade to `@types/react-dom@18.3.x` + preload(imgAttributes.src, opts) + return null + } + + return ( + + + + ) +} + export const Image = forwardRef( (props, forwardedRef) => { + const pagesRouter = useContext(RouterContext) + // We're in the app directory if there is no pages router. + const isAppRouter = !pagesRouter + const configContext = useContext(ImageConfigContext) const config = useMemo(() => { const c = configEnv || configContext || imageConfigDefault @@ -352,29 +403,10 @@ export const Image = forwardRef( /> } {imgMeta.priority ? ( - // Note how we omit the `href` attribute, as it would only be relevant - // for browsers that do not support `imagesrcset`, and in those cases - // it would likely cause the incorrect image to be preloaded. - // - // https://html.spec.whatwg.org/multipage/semantics.html#attr-link-imagesrcset - - - + ) : null} ) diff --git a/test/integration/next-image-new/app-dir/test/index.test.ts b/test/integration/next-image-new/app-dir/test/index.test.ts index cd15fcda21c80..711b329b0dc56 100644 --- a/test/integration/next-image-new/app-dir/test/index.test.ts +++ b/test/integration/next-image-new/app-dir/test/index.test.ts @@ -101,8 +101,7 @@ function runTests(mode) { } }) - // TODO: need to add to app dir - it.skip('should preload priority images', async () => { + it.only('should preload priority images', async () => { let browser try { browser = await webdriver(appPort, '/priority') @@ -125,7 +124,15 @@ function runTests(mode) { const fetchpriority = await link.getAttribute('fetchpriority') const imagesrcset = await link.getAttribute('imagesrcset') const imagesizes = await link.getAttribute('imagesizes') - entries.push({ fetchpriority, imagesrcset, imagesizes }) + const crossorigin = await link.getAttribute('crossorigin') + const referrerPolicy = await link.getAttribute('referrerPolicy') + entries.push({ + fetchpriority, + imagesrcset, + imagesizes, + crossorigin, + referrerPolicy, + }) } expect( @@ -139,6 +146,8 @@ function runTests(mode) { imagesizes: '', imagesrcset: '/_next/image?url=%2Ftest.jpg&w=640&q=75 1x, /_next/image?url=%2Ftest.jpg&w=828&q=75 2x', + crossorigin: 'anonymous', + referrerPolicy: '', }) expect( @@ -152,6 +161,23 @@ function runTests(mode) { imagesizes: '100vw', imagesrcset: '/_next/image?url=%2Fwide.png&w=640&q=75 640w, /_next/image?url=%2Fwide.png&w=750&q=75 750w, /_next/image?url=%2Fwide.png&w=828&q=75 828w, /_next/image?url=%2Fwide.png&w=1080&q=75 1080w, /_next/image?url=%2Fwide.png&w=1200&q=75 1200w, /_next/image?url=%2Fwide.png&w=1920&q=75 1920w, /_next/image?url=%2Fwide.png&w=2048&q=75 2048w, /_next/image?url=%2Fwide.png&w=3840&q=75 3840w', + crossorigin: '', + referrerPolicy: '', + }) + + expect( + entries.find( + (item) => + item.imagesrcset === + '/_next/image?url=%2Ftest.png&w=640&q=75 1x, /_next/image?url=%2Ftest.png&w=828&q=75 2x' + ) + ).toEqual({ + fetchpriority: 'high', + imagesizes: '', + imagesrcset: + '/_next/image?url=%2Ftest.png&w=640&q=75 1x, /_next/image?url=%2Ftest.png&w=828&q=75 2x', + crossorigin: '', + referrerPolicy: 'no-referrer', }) // When priority={true}, we should _not_ set loading="lazy" @@ -197,20 +223,6 @@ function runTests(mode) { /was detected as the Largest Contentful Paint/gm ) expect(warnings).not.toMatch(/React does not recognize the (.+) prop/gm) - - // should preload with crossorigin - expect( - await browser.elementsByCss( - 'link[rel=preload][as=image][crossorigin=anonymous][imagesrcset*="test.jpg"]' - ) - ).toHaveLength(1) - - // should preload with referrerpolicy - expect( - await browser.elementsByCss( - 'link[rel=preload][as=image][referrerpolicy="no-referrer"][imagesrcset*="test.png"]' - ) - ).toHaveLength(1) } finally { if (browser) { await browser.close() diff --git a/test/integration/next-image-new/default/test/index.test.ts b/test/integration/next-image-new/default/test/index.test.ts index 043a78640b505..f6e2b700c443b 100644 --- a/test/integration/next-image-new/default/test/index.test.ts +++ b/test/integration/next-image-new/default/test/index.test.ts @@ -102,7 +102,7 @@ function runTests(mode) { } }) - it('should preload priority images', async () => { + it.only('should preload priority images', async () => { let browser try { browser = await webdriver(appPort, '/priority') @@ -125,8 +125,17 @@ function runTests(mode) { const fetchpriority = await link.getAttribute('fetchpriority') const imagesrcset = await link.getAttribute('imagesrcset') const imagesizes = await link.getAttribute('imagesizes') - entries.push({ fetchpriority, imagesrcset, imagesizes }) + const crossorigin = await link.getAttribute('crossorigin') + const referrerPolicy = await link.getAttribute('referrerPolicy') + entries.push({ + fetchpriority, + imagesrcset, + imagesizes, + crossorigin, + referrerPolicy, + }) } + expect( entries.find( (item) => @@ -138,6 +147,8 @@ function runTests(mode) { imagesizes: '', imagesrcset: '/_next/image?url=%2Ftest.jpg&w=640&q=75 1x, /_next/image?url=%2Ftest.jpg&w=828&q=75 2x', + crossorigin: 'anonymous', + referrerPolicy: '', }) expect( @@ -151,6 +162,23 @@ function runTests(mode) { imagesizes: '100vw', imagesrcset: '/_next/image?url=%2Fwide.png&w=640&q=75 640w, /_next/image?url=%2Fwide.png&w=750&q=75 750w, /_next/image?url=%2Fwide.png&w=828&q=75 828w, /_next/image?url=%2Fwide.png&w=1080&q=75 1080w, /_next/image?url=%2Fwide.png&w=1200&q=75 1200w, /_next/image?url=%2Fwide.png&w=1920&q=75 1920w, /_next/image?url=%2Fwide.png&w=2048&q=75 2048w, /_next/image?url=%2Fwide.png&w=3840&q=75 3840w', + crossorigin: '', + referrerPolicy: '', + }) + + expect( + entries.find( + (item) => + item.imagesrcset === + '/_next/image?url=%2Ftest.png&w=640&q=75 1x, /_next/image?url=%2Ftest.png&w=828&q=75 2x' + ) + ).toEqual({ + fetchpriority: 'high', + imagesizes: '', + imagesrcset: + '/_next/image?url=%2Ftest.png&w=640&q=75 1x, /_next/image?url=%2Ftest.png&w=828&q=75 2x', + crossorigin: '', + referrerPolicy: 'no-referrer', }) // When priority={true}, we should _not_ set loading="lazy" @@ -196,22 +224,6 @@ function runTests(mode) { /was detected as the Largest Contentful Paint/gm ) expect(warnings).not.toMatch(/React does not recognize the (.+) prop/gm) - - // should preload with crossorigin - expect( - ( - await browser.elementsByCss( - 'link[rel=preload][as=image][crossorigin=anonymous][imagesrcset*="test.jpg"]' - ) - ).length - ).toBeGreaterThanOrEqual(1) - - // should preload with referrerpolicy - expect( - await browser.elementsByCss( - 'link[rel=preload][as=image][referrerpolicy="no-referrer"][imagesrcset*="test.png"]' - ) - ).toHaveLength(1) } finally { if (browser) { await browser.close() From c8475e0f0260ef031c1328a4d9f27782c40e6c79 Mon Sep 17 00:00:00 2001 From: Steven Date: Mon, 10 Jul 2023 12:56:00 -0400 Subject: [PATCH 2/7] Update tests --- test/integration/next-image-new/app-dir/app/priority/page.js | 2 +- test/integration/next-image-new/app-dir/test/index.test.ts | 4 ++-- test/integration/next-image-new/default/pages/priority.js | 2 +- test/integration/next-image-new/default/test/index.test.ts | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/integration/next-image-new/app-dir/app/priority/page.js b/test/integration/next-image-new/app-dir/app/priority/page.js index deeb14d41d2d1..ce425dba13660 100644 --- a/test/integration/next-image-new/app-dir/app/priority/page.js +++ b/test/integration/next-image-new/app-dir/app/priority/page.js @@ -17,7 +17,7 @@ const Page = () => { priority id="basic-image-crossorigin" alt="basic-image-crossorigin" - src="/test.jpg" + src="/test.webp" width="400" height="400" crossOrigin="anonymous" diff --git a/test/integration/next-image-new/app-dir/test/index.test.ts b/test/integration/next-image-new/app-dir/test/index.test.ts index 711b329b0dc56..ae614549fe415 100644 --- a/test/integration/next-image-new/app-dir/test/index.test.ts +++ b/test/integration/next-image-new/app-dir/test/index.test.ts @@ -139,13 +139,13 @@ function runTests(mode) { entries.find( (item) => item.imagesrcset === - '/_next/image?url=%2Ftest.jpg&w=640&q=75 1x, /_next/image?url=%2Ftest.jpg&w=828&q=75 2x' + '/_next/image?url=%2Ftest.webp&w=640&q=75 1x, /_next/image?url=%2Ftest.webp&w=828&q=75 2x' ) ).toEqual({ fetchpriority: 'high', imagesizes: '', imagesrcset: - '/_next/image?url=%2Ftest.jpg&w=640&q=75 1x, /_next/image?url=%2Ftest.jpg&w=828&q=75 2x', + '/_next/image?url=%2Ftest.webp&w=640&q=75 1x, /_next/image?url=%2Ftest.webp&w=828&q=75 2x', crossorigin: 'anonymous', referrerPolicy: '', }) diff --git a/test/integration/next-image-new/default/pages/priority.js b/test/integration/next-image-new/default/pages/priority.js index deeb14d41d2d1..ce425dba13660 100644 --- a/test/integration/next-image-new/default/pages/priority.js +++ b/test/integration/next-image-new/default/pages/priority.js @@ -17,7 +17,7 @@ const Page = () => { priority id="basic-image-crossorigin" alt="basic-image-crossorigin" - src="/test.jpg" + src="/test.webp" width="400" height="400" crossOrigin="anonymous" diff --git a/test/integration/next-image-new/default/test/index.test.ts b/test/integration/next-image-new/default/test/index.test.ts index f6e2b700c443b..53bdbb3b914e0 100644 --- a/test/integration/next-image-new/default/test/index.test.ts +++ b/test/integration/next-image-new/default/test/index.test.ts @@ -140,13 +140,13 @@ function runTests(mode) { entries.find( (item) => item.imagesrcset === - '/_next/image?url=%2Ftest.jpg&w=640&q=75 1x, /_next/image?url=%2Ftest.jpg&w=828&q=75 2x' + '/_next/image?url=%2Ftest.webp&w=640&q=75 1x, /_next/image?url=%2Ftest.webp&w=828&q=75 2x' ) ).toEqual({ fetchpriority: 'high', imagesizes: '', imagesrcset: - '/_next/image?url=%2Ftest.jpg&w=640&q=75 1x, /_next/image?url=%2Ftest.jpg&w=828&q=75 2x', + '/_next/image?url=%2Ftest.webp&w=640&q=75 1x, /_next/image?url=%2Ftest.webp&w=828&q=75 2x', crossorigin: 'anonymous', referrerPolicy: '', }) From 390d56657327e4c7c709b26f0d955e2e57bd428e Mon Sep 17 00:00:00 2001 From: Steven Date: Mon, 10 Jul 2023 14:44:18 -0400 Subject: [PATCH 3/7] Fix `crossorigin` tests --- .../integration/next-image-new/app-dir/app/priority/page.js | 6 +++--- test/integration/next-image-new/app-dir/test/index.test.ts | 6 +++--- test/integration/next-image-new/default/pages/priority.js | 6 +++--- test/integration/next-image-new/default/test/index.test.ts | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/test/integration/next-image-new/app-dir/app/priority/page.js b/test/integration/next-image-new/app-dir/app/priority/page.js index ce425dba13660..f1501e0a9917b 100644 --- a/test/integration/next-image-new/app-dir/app/priority/page.js +++ b/test/integration/next-image-new/app-dir/app/priority/page.js @@ -20,7 +20,7 @@ const Page = () => { src="/test.webp" width="400" height="400" - crossOrigin="anonymous" + crossOrigin="use-credentials" > { id="load-eager" alt="load-eager" src="/test.png" - width="400" - height="400" + width="200" + height="200" > { src="/test.webp" width="400" height="400" - crossOrigin="anonymous" + crossOrigin="use-credentials" > { id="load-eager" alt="load-eager" src="/test.png" - width="400" - height="400" + width="200" + height="200" > Date: Thu, 13 Jul 2023 11:41:50 -0400 Subject: [PATCH 4/7] Remove `it.only()` --- test/integration/next-image-new/app-dir/test/index.test.ts | 2 +- test/integration/next-image-new/default/test/index.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/next-image-new/app-dir/test/index.test.ts b/test/integration/next-image-new/app-dir/test/index.test.ts index 9f7951eab8e3d..359915675577b 100644 --- a/test/integration/next-image-new/app-dir/test/index.test.ts +++ b/test/integration/next-image-new/app-dir/test/index.test.ts @@ -101,7 +101,7 @@ function runTests(mode) { } }) - it.only('should preload priority images', async () => { + it('should preload priority images', async () => { let browser try { browser = await webdriver(appPort, '/priority') diff --git a/test/integration/next-image-new/default/test/index.test.ts b/test/integration/next-image-new/default/test/index.test.ts index 24276a49d38b1..88f33c65ba526 100644 --- a/test/integration/next-image-new/default/test/index.test.ts +++ b/test/integration/next-image-new/default/test/index.test.ts @@ -102,7 +102,7 @@ function runTests(mode) { } }) - it.only('should preload priority images', async () => { + it('should preload priority images', async () => { let browser try { browser = await webdriver(appPort, '/priority') From d3c3b558afc99b231ed924aab04bf61d39732d3f Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 14 Jul 2023 10:43:29 -0400 Subject: [PATCH 5/7] Fix unit test --- test/unit/next-image-new.test.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/test/unit/next-image-new.test.ts b/test/unit/next-image-new.test.ts index f2024db4022ee..60f8ea809c6a9 100644 --- a/test/unit/next-image-new.test.ts +++ b/test/unit/next-image-new.test.ts @@ -1,9 +1,13 @@ /* eslint-env jest */ import React from 'react' -import ReactDOM from 'react-dom/server' +import ReactDOMServer from 'react-dom/server' import Image from 'next/image' import cheerio from 'cheerio' +// Since this unit test doesn't check the result of +// ReactDOM.preload(), we can turn it into a noop. +jest.mock('react-dom', () => ({ preload: () => null })) + describe('Image rendering', () => { it('should render Image on its own', async () => { const element = React.createElement(Image, { @@ -14,7 +18,7 @@ describe('Image rendering', () => { height: 100, loading: 'eager', }) - const html = ReactDOM.renderToString(element) + const html = ReactDOMServer.renderToString(element) const $ = cheerio.load(html) const img = $('#unit-image') // order matters here @@ -54,9 +58,9 @@ describe('Image rendering', () => { width: 100, height: 100, }) - const $ = cheerio.load(ReactDOM.renderToString(element)) - const $2 = cheerio.load(ReactDOM.renderToString(element2)) - const $lazy = cheerio.load(ReactDOM.renderToString(elementLazy)) + const $ = cheerio.load(ReactDOMServer.renderToString(element)) + const $2 = cheerio.load(ReactDOMServer.renderToString(element2)) + const $lazy = cheerio.load(ReactDOMServer.renderToString(elementLazy)) expect($('noscript').length).toBe(0) expect($2('noscript').length).toBe(0) expect($lazy('noscript').length).toBe(0) @@ -90,9 +94,9 @@ describe('Image rendering', () => { blurDataURL: 'data:image/png;base64', priority: true, }) - const $1 = cheerio.load(ReactDOM.renderToString(element1)) - const $2 = cheerio.load(ReactDOM.renderToString(element2)) - const $3 = cheerio.load(ReactDOM.renderToString(element3)) + const $1 = cheerio.load(ReactDOMServer.renderToString(element1)) + const $2 = cheerio.load(ReactDOMServer.renderToString(element2)) + const $3 = cheerio.load(ReactDOMServer.renderToString(element3)) expect($1('noscript').length).toBe(0) expect($2('noscript').length).toBe(0) expect($3('noscript').length).toBe(0) From 213d3b010493804219d1e1f58e164135512d7913 Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 14 Jul 2023 10:45:13 -0400 Subject: [PATCH 6/7] Fix camelCase referrerPolicy --- .../next-image-new/app-dir/test/index.test.ts | 6 +++--- .../next-image-new/default/test/index.test.ts | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/integration/next-image-new/app-dir/test/index.test.ts b/test/integration/next-image-new/app-dir/test/index.test.ts index 359915675577b..ccc2af01cce1c 100644 --- a/test/integration/next-image-new/app-dir/test/index.test.ts +++ b/test/integration/next-image-new/app-dir/test/index.test.ts @@ -125,13 +125,13 @@ function runTests(mode) { const imagesrcset = await link.getAttribute('imagesrcset') const imagesizes = await link.getAttribute('imagesizes') const crossorigin = await link.getAttribute('crossorigin') - const referrerPolicy = await link.getAttribute('referrerPolicy') + const referrerpolicy = await link.getAttribute('referrerPolicy') entries.push({ fetchpriority, imagesrcset, imagesizes, crossorigin, - referrerPolicy, + referrerpolicy, }) } @@ -147,7 +147,7 @@ function runTests(mode) { imagesrcset: '/_next/image?url=%2Ftest.webp&w=640&q=75 1x, /_next/image?url=%2Ftest.webp&w=828&q=75 2x', crossorigin: 'use-credentials', - referrerPolicy: '', + referrerpolicy: '', }) expect( diff --git a/test/integration/next-image-new/default/test/index.test.ts b/test/integration/next-image-new/default/test/index.test.ts index 88f33c65ba526..13f85437fcdcc 100644 --- a/test/integration/next-image-new/default/test/index.test.ts +++ b/test/integration/next-image-new/default/test/index.test.ts @@ -126,13 +126,13 @@ function runTests(mode) { const imagesrcset = await link.getAttribute('imagesrcset') const imagesizes = await link.getAttribute('imagesizes') const crossorigin = await link.getAttribute('crossorigin') - const referrerPolicy = await link.getAttribute('referrerPolicy') + const referrerpolicy = await link.getAttribute('referrerPolicy') entries.push({ fetchpriority, imagesrcset, imagesizes, crossorigin, - referrerPolicy, + referrerpolicy, }) } @@ -148,7 +148,7 @@ function runTests(mode) { imagesrcset: '/_next/image?url=%2Ftest.webp&w=640&q=75 1x, /_next/image?url=%2Ftest.webp&w=828&q=75 2x', crossorigin: 'use-credentials', - referrerPolicy: '', + referrerpolicy: '', }) expect( @@ -163,7 +163,7 @@ function runTests(mode) { imagesrcset: '/_next/image?url=%2Fwide.png&w=640&q=75 640w, /_next/image?url=%2Fwide.png&w=750&q=75 750w, /_next/image?url=%2Fwide.png&w=828&q=75 828w, /_next/image?url=%2Fwide.png&w=1080&q=75 1080w, /_next/image?url=%2Fwide.png&w=1200&q=75 1200w, /_next/image?url=%2Fwide.png&w=1920&q=75 1920w, /_next/image?url=%2Fwide.png&w=2048&q=75 2048w, /_next/image?url=%2Fwide.png&w=3840&q=75 3840w', crossorigin: '', - referrerPolicy: '', + referrerpolicy: '', }) expect( @@ -178,7 +178,7 @@ function runTests(mode) { imagesrcset: '/_next/image?url=%2Ftest.png&w=640&q=75 1x, /_next/image?url=%2Ftest.png&w=828&q=75 2x', crossorigin: '', - referrerPolicy: 'no-referrer', + referrerpolicy: 'no-referrer', }) // When priority={true}, we should _not_ set loading="lazy" From 882050bff044504795a9e0fd77349009c9b1d1f4 Mon Sep 17 00:00:00 2001 From: Steven Date: Fri, 14 Jul 2023 10:49:14 -0400 Subject: [PATCH 7/7] Only expect error on preload opts --- packages/next/src/client/image-component.tsx | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/next/src/client/image-component.tsx b/packages/next/src/client/image-component.tsx index a056c447c8954..b113c0bb0c209 100644 --- a/packages/next/src/client/image-component.tsx +++ b/packages/next/src/client/image-component.tsx @@ -322,8 +322,11 @@ function ImagePreload({ if (isAppRouter) { // See https://github.com/facebook/react/pull/26940 - // @ts-expect-error TODO: upgrade to `@types/react-dom@18.3.x` - preload(imgAttributes.src, opts) + preload( + imgAttributes.src, + // @ts-expect-error TODO: upgrade to `@types/react-dom@18.3.x` + opts + ) return null }