From b37983a130ffa0dac6d7629590027a574de0a426 Mon Sep 17 00:00:00 2001 From: Steven Date: Sun, 25 Oct 2020 18:50:10 -0400 Subject: [PATCH 1/3] Fix image srcset --- packages/next/client/image.tsx | 137 +++++++++++------- .../image-component/basic/pages/index.js | 2 +- .../image-component/basic/pages/lazy.js | 2 +- .../image-component/basic/test/index.test.js | 14 +- .../default/test/index.test.js | 2 +- 5 files changed, 95 insertions(+), 62 deletions(-) diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index a38b40e1cafa0..8b551b16abf61 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -104,29 +104,45 @@ function callLoader(loaderProps: CallLoaderProps) { type SrcSetData = { src: string - widths: number[] - quality?: string + unoptimized: boolean + width: number | undefined + quality: string | undefined } -function generateSrcSet({ src, widths, quality }: SrcSetData): string { +function generateSrcSet({ + src, + unoptimized, + width, + quality, +}: SrcSetData): string | undefined { // At each breakpoint, generate an image url using the loader, such as: // ' www.example.com/foo.jpg?w=480 480w, ' + if (unoptimized) { + return undefined + } + let widths = configSizes + if (typeof width === 'number') { + widths = configSizes.filter((size) => size <= width) + if (widths.length === 0) { + widths = [configSizes[0]] + } + } return widths - .map((width: number) => `${callLoader({ src, width, quality })} ${width}w`) + .map((w) => `${callLoader({ src, width: w, quality })} ${w}w`) .join(', ') } type PreloadData = { src: string - widths: number[] + unoptimized: boolean + width: number | undefined sizes?: string - unoptimized?: boolean quality?: string } function generatePreload({ src, - widths, + width, unoptimized = false, sizes, quality, @@ -142,13 +158,23 @@ function generatePreload({ as="image" href={computeSrc(src, unoptimized, quality)} // @ts-ignore: imagesrcset and imagesizes not yet in the link element type - imagesrcset={generateSrcSet({ src, widths, quality })} + imagesrcset={generateSrcSet({ src, unoptimized, width, quality })} imagesizes={sizes} /> ) } +function getInt(x: unknown): number | undefined { + if (typeof x === 'number') { + return x + } + if (typeof x === 'string') { + return parseInt(x, 10) + } + return undefined +} + export default function Image({ src, sizes, @@ -165,6 +191,13 @@ export default function Image({ const thisEl = useRef(null) if (process.env.NODE_ENV !== 'production') { + if (!src) { + throw new Error( + `Image is missing required "src" property. Make sure you pass "src" in props to the \`next/image\` component. Received: ${JSON.stringify( + { width, height, quality, unsized } + )}` + ) + } if (!VALID_LOADING_VALUES.includes(loading)) { throw new Error( `Image with src "${src}" has invalid "loading" property. Provided "${loading}" should be one of ${VALID_LOADING_VALUES.map( @@ -200,58 +233,23 @@ export default function Image({ } }, [thisEl, lazy]) - // Generate attribute values - const imgSrc = computeSrc(src, unoptimized, quality) - const imgSrcSet = !unoptimized - ? generateSrcSet({ - src, - widths: configSizes, - quality, - }) - : undefined - - let imgAttributes: - | { - src: string - srcSet?: string - } - | { - 'data-src': string - 'data-srcset'?: string - } - if (!lazy) { - imgAttributes = { - src: imgSrc, - } - if (imgSrcSet) { - imgAttributes.srcSet = imgSrcSet - } - } else { - imgAttributes = { - 'data-src': imgSrc, - } - if (imgSrcSet) { - imgAttributes['data-srcset'] = imgSrcSet - } - className = className ? className + ' __lazy' : '__lazy' - } - + let widthInt = getInt(width) + let heightInt = getInt(height) let divStyle: React.CSSProperties | undefined let imgStyle: React.CSSProperties | undefined let wrapperStyle: React.CSSProperties | undefined if ( - typeof height !== 'undefined' && - typeof width !== 'undefined' && + typeof widthInt !== 'undefined' && + typeof heightInt !== 'undefined' && !unsized ) { // // - const quotient = - parseInt(height as string, 10) / parseInt(width as string, 10) + const quotient = heightInt / widthInt const ratio = isNaN(quotient) ? 1 : quotient * 100 wrapperStyle = { maxWidth: '100%', - width, + width: widthInt, } divStyle = { position: 'relative', @@ -266,8 +264,8 @@ export default function Image({ width: '100%', } } else if ( - typeof height === 'undefined' && - typeof width === 'undefined' && + typeof widthInt === 'undefined' && + typeof heightInt === 'undefined' && unsized ) { // @@ -288,6 +286,41 @@ export default function Image({ } } + // Generate attribute values + const imgSrc = computeSrc(src, unoptimized, quality) + const imgSrcSet = generateSrcSet({ + src, + width: widthInt, + unoptimized, + quality, + }) + + let imgAttributes: + | { + src: string + srcSet?: string + } + | { + 'data-src': string + 'data-srcset'?: string + } + if (!lazy) { + imgAttributes = { + src: imgSrc, + } + if (imgSrcSet) { + imgAttributes.srcSet = imgSrcSet + } + } else { + imgAttributes = { + 'data-src': imgSrc, + } + if (imgSrcSet) { + imgAttributes['data-srcset'] = imgSrcSet + } + className = className ? className + ' __lazy' : '__lazy' + } + // No need to add preloads on the client side--by the time the application is hydrated, // it's too late for preloads const shouldPreload = priority && typeof window === 'undefined' @@ -298,7 +331,7 @@ export default function Image({ {shouldPreload ? generatePreload({ src, - widths: configSizes, + width: widthInt, unoptimized, sizes, quality, diff --git a/test/integration/image-component/basic/pages/index.js b/test/integration/image-component/basic/pages/index.js index 0ad8d8e1ac445..6692de7fd4078 100644 --- a/test/integration/image-component/basic/pages/index.js +++ b/test/integration/image-component/basic/pages/index.js @@ -19,7 +19,7 @@ const Page = () => { data-demo="demo-value" src="bar.jpg" loading="eager" - width={300} + width={1024} height={400} /> { id="lazy-top" src="foo1.jpg" height={400} - width={300} + width={1024} loading="lazy" >
diff --git a/test/integration/image-component/basic/test/index.test.js b/test/integration/image-component/basic/test/index.test.js index bda3f67eea792..a9ea9f4de908b 100644 --- a/test/integration/image-component/basic/test/index.test.js +++ b/test/integration/image-component/basic/test/index.test.js @@ -44,15 +44,13 @@ function runTests() { it('should add a srcset based on the loader', async () => { expect( await browser.elementById('basic-image').getAttribute('srcset') - ).toBe( - 'https://example.com/myaccount/foo.jpg?auto=format&w=480&q=60 480w, https://example.com/myaccount/foo.jpg?auto=format&w=1024&q=60 1024w, https://example.com/myaccount/foo.jpg?auto=format&w=1600&q=60 1600w' - ) + ).toBe('https://example.com/myaccount/foo.jpg?auto=format&w=480&q=60 480w') }) it('should add a srcset even with preceding slash in prop', async () => { expect( await browser.elementById('preceding-slash-image').getAttribute('srcset') ).toBe( - 'https://example.com/myaccount/fooslash.jpg?auto=format&w=480 480w, https://example.com/myaccount/fooslash.jpg?auto=format&w=1024 1024w, https://example.com/myaccount/fooslash.jpg?auto=format&w=1600 1600w' + 'https://example.com/myaccount/fooslash.jpg?auto=format&w=480 480w, https://example.com/myaccount/fooslash.jpg?auto=format&w=1024 1024w' ) }) it('should support the unoptimized attribute', async () => { @@ -73,7 +71,7 @@ function lazyLoadingTests() { 'https://example.com/myaccount/foo1.jpg?auto=format&w=1600' ) expect(await browser.elementById('lazy-top').getAttribute('srcset')).toBe( - 'https://example.com/myaccount/foo1.jpg?auto=format&w=480 480w, https://example.com/myaccount/foo1.jpg?auto=format&w=1024 1024w, https://example.com/myaccount/foo1.jpg?auto=format&w=1600 1600w' + 'https://example.com/myaccount/foo1.jpg?auto=format&w=480 480w, https://example.com/myaccount/foo1.jpg?auto=format&w=1024 1024w' ) }) it('should not have loaded the second image immediately', async () => { @@ -105,7 +103,7 @@ function lazyLoadingTests() { await check(() => { return browser.elementById('lazy-mid').getAttribute('srcset') - }, 'https://example.com/myaccount/foo2.jpg?auto=format&w=480 480w, https://example.com/myaccount/foo2.jpg?auto=format&w=1024 1024w, https://example.com/myaccount/foo2.jpg?auto=format&w=1600 1600w') + }, 'https://example.com/myaccount/foo2.jpg?auto=format&w=480 480w') }) it('should not have loaded the third image after scrolling down', async () => { expect( @@ -153,7 +151,9 @@ function lazyLoadingTests() { ).toBe('https://example.com/myaccount/foo4.jpg?auto=format&w=1600') expect( await browser.elementById('lazy-without-attribute').getAttribute('srcset') - ).toBeTruthy() + ).toBe( + 'https://example.com/myaccount/foo4.jpg?auto=format&w=480&q=60 480w, https://example.com/myaccount/foo4.jpg?auto=format&w=1024&q=60 1024w, https://example.com/myaccount/foo4.jpg?auto=format&w=1600&q=60 1600w' + ) }) it('should load the fifth image eagerly, without scrolling', async () => { diff --git a/test/integration/image-component/default/test/index.test.js b/test/integration/image-component/default/test/index.test.js index 8af093b9240c4..c2c62f1412780 100644 --- a/test/integration/image-component/default/test/index.test.js +++ b/test/integration/image-component/default/test/index.test.js @@ -88,7 +88,7 @@ function runTests(mode) { await hasRedbox(browser) expect(await getRedboxHeader(browser)).toContain( - 'Next Image Optimization requires src to be provided. Make sure you pass them as props to the `next/image` component. Received: {"width":1200}' + 'Image is missing required "src" property. Make sure you pass "src" in props to the `next/image` component. Received: {"width":200}' ) }) From a2df7f65401b3a48111e521054f89c9d1f9372d4 Mon Sep 17 00:00:00 2001 From: Steven Date: Mon, 26 Oct 2020 09:30:07 -0400 Subject: [PATCH 2/3] Fix tests and fallback image --- packages/next/client/image.tsx | 16 +++++++++---- .../image-component/basic/next.config.js | 2 +- .../image-component/basic/pages/index.js | 1 - .../image-component/basic/pages/lazy.js | 4 ++-- .../image-component/basic/test/index.test.js | 24 +++++++++---------- 5 files changed, 26 insertions(+), 21 deletions(-) diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index 8b551b16abf61..6ef8fcca729a6 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -42,7 +42,6 @@ const { domains: configDomains, } = imageData configSizes.sort((a, b) => a - b) // smallest to largest -const largestSize = configSizes[configSizes.length - 1] let cachedObserver: IntersectionObserver const IntersectionObserver = @@ -83,12 +82,21 @@ function getObserver(): IntersectionObserver | undefined { function computeSrc( src: string, unoptimized: boolean, + width: number | undefined, quality?: string ): string { if (unoptimized) { return src } - return callLoader({ src, width: largestSize, quality }) + let widths = configSizes + if (typeof width === 'number') { + widths = configSizes.filter((size) => size <= width) + if (widths.length === 0) { + widths = [configSizes[0]] + } + } + const largest = widths[widths.length - 1] + return callLoader({ src, width: largest, quality }) } type CallLoaderProps = { @@ -156,7 +164,7 @@ function generatePreload({ { width={300} height={400} /> - { id="lazy-without-attribute" src="foo4.jpg" height={400} - width={300} + width={1500} >
{ src="foo5.jpg" loading="eager" height={400} - width={300} + width={2500} > ) diff --git a/test/integration/image-component/basic/test/index.test.js b/test/integration/image-component/basic/test/index.test.js index a9ea9f4de908b..9e9198cdac42b 100644 --- a/test/integration/image-component/basic/test/index.test.js +++ b/test/integration/image-component/basic/test/index.test.js @@ -33,13 +33,13 @@ function runTests() { }) it('should modify src with the loader', async () => { expect(await browser.elementById('basic-image').getAttribute('src')).toBe( - 'https://example.com/myaccount/foo.jpg?auto=format&w=1600&q=60' + 'https://example.com/myaccount/foo.jpg?auto=format&w=480&q=60' ) }) it('should correctly generate src even if preceding slash is included in prop', async () => { expect( await browser.elementById('preceding-slash-image').getAttribute('src') - ).toBe('https://example.com/myaccount/fooslash.jpg?auto=format&w=1600') + ).toBe('https://example.com/myaccount/fooslash.jpg?auto=format&w=480') }) it('should add a srcset based on the loader', async () => { expect( @@ -49,9 +49,7 @@ function runTests() { it('should add a srcset even with preceding slash in prop', async () => { expect( await browser.elementById('preceding-slash-image').getAttribute('srcset') - ).toBe( - 'https://example.com/myaccount/fooslash.jpg?auto=format&w=480 480w, https://example.com/myaccount/fooslash.jpg?auto=format&w=1024 1024w' - ) + ).toBe('https://example.com/myaccount/fooslash.jpg?auto=format&w=480 480w') }) it('should support the unoptimized attribute', async () => { expect( @@ -68,7 +66,7 @@ function runTests() { function lazyLoadingTests() { it('should have loaded the first image immediately', async () => { expect(await browser.elementById('lazy-top').getAttribute('src')).toBe( - 'https://example.com/myaccount/foo1.jpg?auto=format&w=1600' + 'https://example.com/myaccount/foo1.jpg?auto=format&w=1024' ) expect(await browser.elementById('lazy-top').getAttribute('srcset')).toBe( 'https://example.com/myaccount/foo1.jpg?auto=format&w=480 480w, https://example.com/myaccount/foo1.jpg?auto=format&w=1024 1024w' @@ -99,7 +97,7 @@ function lazyLoadingTests() { await check(() => { return browser.elementById('lazy-mid').getAttribute('src') - }, 'https://example.com/myaccount/foo2.jpg?auto=format&w=1600') + }, 'https://example.com/myaccount/foo2.jpg?auto=format&w=480') await check(() => { return browser.elementById('lazy-mid').getAttribute('srcset') @@ -148,17 +146,17 @@ function lazyLoadingTests() { await waitFor(200) expect( await browser.elementById('lazy-without-attribute').getAttribute('src') - ).toBe('https://example.com/myaccount/foo4.jpg?auto=format&w=1600') + ).toBe('https://example.com/myaccount/foo4.jpg?auto=format&w=1024') expect( await browser.elementById('lazy-without-attribute').getAttribute('srcset') ).toBe( - 'https://example.com/myaccount/foo4.jpg?auto=format&w=480&q=60 480w, https://example.com/myaccount/foo4.jpg?auto=format&w=1024&q=60 1024w, https://example.com/myaccount/foo4.jpg?auto=format&w=1600&q=60 1600w' + 'https://example.com/myaccount/foo4.jpg?auto=format&w=480 480w, https://example.com/myaccount/foo4.jpg?auto=format&w=1024 1024w' ) }) it('should load the fifth image eagerly, without scrolling', async () => { expect(await browser.elementById('eager-loading').getAttribute('src')).toBe( - 'https://example.com/myaccount/foo5.jpg?auto=format&w=1600' + 'https://example.com/myaccount/foo5.jpg?auto=format&w=2000' ) expect( await browser.elementById('eager-loading').getAttribute('srcset') @@ -198,14 +196,14 @@ describe('Image Component Tests', () => { it('should add a preload tag for a priority image', async () => { expect( await hasPreloadLinkMatchingUrl( - 'https://example.com/myaccount/withpriority.png?auto=format&w=1600' + 'https://example.com/myaccount/withpriority.png?auto=format&w=480&q=60' ) ).toBe(true) }) it('should add a preload tag for a priority image with preceding slash', async () => { expect( await hasPreloadLinkMatchingUrl( - 'https://example.com/myaccount/fooslash.jpg?auto=format&w=1600' + 'https://example.com/myaccount/fooslash.jpg?auto=format&w=480' ) ).toBe(true) }) @@ -219,7 +217,7 @@ describe('Image Component Tests', () => { it('should add a preload tag for a priority image, with quality', async () => { expect( await hasPreloadLinkMatchingUrl( - 'https://example.com/myaccount/withpriority.png?auto=format&w=1600&q=60' + 'https://example.com/myaccount/withpriority.png?auto=format&w=480&q=60' ) ).toBe(true) }) From 77b015fd401d3f529f2df774941b0efe028f5dca Mon Sep 17 00:00:00 2001 From: Steven Date: Mon, 26 Oct 2020 09:46:26 -0400 Subject: [PATCH 3/3] Round up when generating srcset --- packages/next/client/image.tsx | 31 ++++++++++--------- .../image-component/basic/pages/lazy.js | 4 +-- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index 6ef8fcca729a6..457b2211d1869 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -79,6 +79,20 @@ function getObserver(): IntersectionObserver | undefined { )) } +function getWidthsFromConfig(width: number | undefined) { + if (typeof width !== 'number') { + return configSizes + } + const widths: number[] = [] + for (let size of configSizes) { + widths.push(size) + if (size >= width) { + break + } + } + return widths +} + function computeSrc( src: string, unoptimized: boolean, @@ -88,13 +102,7 @@ function computeSrc( if (unoptimized) { return src } - let widths = configSizes - if (typeof width === 'number') { - widths = configSizes.filter((size) => size <= width) - if (widths.length === 0) { - widths = [configSizes[0]] - } - } + const widths = getWidthsFromConfig(width) const largest = widths[widths.length - 1] return callLoader({ src, width: largest, quality }) } @@ -128,14 +136,7 @@ function generateSrcSet({ if (unoptimized) { return undefined } - let widths = configSizes - if (typeof width === 'number') { - widths = configSizes.filter((size) => size <= width) - if (widths.length === 0) { - widths = [configSizes[0]] - } - } - return widths + return getWidthsFromConfig(width) .map((w) => `${callLoader({ src, width: w, quality })} ${w}w`) .join(', ') } diff --git a/test/integration/image-component/basic/pages/lazy.js b/test/integration/image-component/basic/pages/lazy.js index c0e10e21ef46a..161b530bc3c3f 100644 --- a/test/integration/image-component/basic/pages/lazy.js +++ b/test/integration/image-component/basic/pages/lazy.js @@ -35,7 +35,7 @@ const Lazy = () => { id="lazy-without-attribute" src="foo4.jpg" height={400} - width={1500} + width={800} >
{ src="foo5.jpg" loading="eager" height={400} - width={2500} + width={1900} > )