From 6d8643e452b499766d5440eea2b22a2eb0772522 Mon Sep 17 00:00:00 2001 From: atcastle Date: Mon, 19 Oct 2020 17:02:14 -0700 Subject: [PATCH 1/4] Fix flaky scroll tests for image component --- test/integration/image-component/basic/test/index.test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/integration/image-component/basic/test/index.test.js b/test/integration/image-component/basic/test/index.test.js index 67315a2b15f89..ffa87e0c6b58f 100644 --- a/test/integration/image-component/basic/test/index.test.js +++ b/test/integration/image-component/basic/test/index.test.js @@ -102,6 +102,7 @@ function lazyLoadingTests() { await browser.eval( `window.scrollTo(0, ${topOfMidImage - (viewportHeight + buffer)})` ) + await waitFor(200) expect(await browser.elementById('lazy-mid').getAttribute('src')).toBe( 'https://example.com/myaccount/foo2.jpg' ) @@ -126,6 +127,7 @@ function lazyLoadingTests() { await browser.eval( `window.scrollTo(0, ${topOfBottomImage - (viewportHeight + buffer)})` ) + await waitFor(200) expect(await browser.elementById('lazy-bottom').getAttribute('src')).toBe( 'https://www.otherhost.com/foo3.jpg' ) From ae4079726e299f66ead63ff06acb1b63739dc100 Mon Sep 17 00:00:00 2001 From: atcastle Date: Mon, 19 Oct 2020 17:39:32 -0700 Subject: [PATCH 2/4] Remove logic for multiple hosts --- packages/next/build/webpack-config.ts | 22 ++----- packages/next/client/image.tsx | 62 +++---------------- .../image-component/basic/next.config.js | 12 +--- .../image-component/basic/test/index.test.js | 12 ---- 4 files changed, 17 insertions(+), 91 deletions(-) diff --git a/packages/next/build/webpack-config.ts b/packages/next/build/webpack-config.ts index 0df798ee80343..25e892125d000 100644 --- a/packages/next/build/webpack-config.ts +++ b/packages/next/build/webpack-config.ts @@ -229,25 +229,13 @@ export default async function getBaseWebpackConfig( } } - if (config.images?.hosts) { - if (!config.images.hosts.default) { - // If the image component is being used, a default host must be provided - throw new Error( - 'If the image configuration property is present in next.config.js, it must have a host named "default"' - ) + // Normalize defined image host to end in slash + if (config.images?.path) { + if (config.images.path[config.images.path.length - 1] !== '/') { + config.images.path += '/' } - Object.values(config.images.hosts).forEach((host: any) => { - if (!host.path) { - throw new Error( - 'All hosts defined in the image configuration property of next.config.js must define a path' - ) - } - // Normalize hosts so all paths have trailing slash - if (host.path[host.path.length - 1] !== '/') { - host.path += '/' - } - }) } + const reactVersion = await getPackageVersion({ cwd: dir, name: 'react' }) const hasReactRefresh: boolean = dev && !isServer const hasJsxRuntime: boolean = diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index 28e5f13579a70..95c84652faa28 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -9,12 +9,6 @@ const loaders: { [key: string]: (props: LoaderProps) => string } = { type ImageData = { sizes?: number[] - hosts: { - [key: string]: { - path: string - loader: string - } - } } type ImageProps = Omit< @@ -22,7 +16,6 @@ type ImageProps = Omit< 'src' | 'srcSet' | 'ref' > & { src: string - host?: string priority?: boolean lazy?: boolean unoptimized?: boolean @@ -66,49 +59,33 @@ function getObserver(): IntersectionObserver | undefined { )) } -function computeSrc(src: string, host: string, unoptimized: boolean): string { +function computeSrc(src: string, unoptimized: boolean): string { if (unoptimized) { return src } - if (!host) { - // No host provided, use default - return callLoader(src, 'default') - } else { - let selectedHost = imageData.hosts[host] - if (!selectedHost) { - if (process.env.NODE_ENV !== 'production') { - console.error( - `Image tag is used specifying host ${host}, but that host is not defined in next.config` - ) - } - return src - } - return callLoader(src, host) - } + return callLoader(src) } -function callLoader(src: string, host: string, width?: number): string { - let loader = loaders[imageData.hosts[host].loader || 'default'] - return loader({ root: imageData.hosts[host].path, src, width }) +function callLoader(src: string, width?: number): string { + let loader = loaders[imageData.loader || 'default'] + return loader({ root: imageData.path, src, width }) } type SrcSetData = { src: string - host: string widths: number[] } -function generateSrcSet({ src, host, widths }: SrcSetData): string { +function generateSrcSet({ src, widths }: SrcSetData): string { // At each breakpoint, generate an image url using the loader, such as: // ' www.example.com/foo.jpg?w=480 480w, ' return widths - .map((width: number) => `${callLoader(src, host, width)} ${width}w`) + .map((width: number) => `${callLoader(src, width)} ${width}w`) .join(', ') } type PreloadData = { src: string - host: string widths: number[] sizes?: string unoptimized?: boolean @@ -116,7 +93,6 @@ type PreloadData = { function generatePreload({ src, - host, widths, unoptimized = false, sizes, @@ -130,9 +106,9 @@ function generatePreload({ @@ -141,7 +117,6 @@ function generatePreload({ export default function Image({ src, - host, sizes, unoptimized = false, priority = false, @@ -152,19 +127,6 @@ export default function Image({ const thisEl = useRef(null) // Sanity Checks: - if (process.env.NODE_ENV !== 'production') { - if (unoptimized && host) { - console.error(`Image tag used specifying both a host and the unoptimized attribute--these are mutually exclusive. - With the unoptimized attribute, no host will be used, so specify an absolute URL.`) - } - } - if (host && !imageData.hosts[host]) { - // If unregistered host is selected, log an error and use the default instead - if (process.env.NODE_ENV !== 'production') { - console.error(`Image host identifier ${host} could not be resolved.`) - } - host = 'default' - } // If priority and lazy are present, log an error and use priority only. if (priority && lazy) { if (process.env.NODE_ENV !== 'production') { @@ -175,8 +137,6 @@ export default function Image({ lazy = false } - host = host || 'default' - // Normalize provided src if (src[0] === '/') { src = src.slice(1) @@ -199,11 +159,10 @@ export default function Image({ }, [thisEl, lazy]) // Generate attribute values - const imgSrc = computeSrc(src, host, unoptimized) + const imgSrc = computeSrc(src, unoptimized) const imgSrcSet = !unoptimized ? generateSrcSet({ src, - host: host, widths: breakpoints, }) : undefined @@ -243,7 +202,6 @@ export default function Image({ {shouldPreload ? generatePreload({ src, - host, widths: breakpoints, unoptimized, sizes, diff --git a/test/integration/image-component/basic/next.config.js b/test/integration/image-component/basic/next.config.js index 8ada091227bd4..8f260dea043fd 100644 --- a/test/integration/image-component/basic/next.config.js +++ b/test/integration/image-component/basic/next.config.js @@ -1,15 +1,7 @@ module.exports = { images: { sizes: [480, 1024, 1600], - hosts: { - default: { - path: 'https://example.com/myaccount/', - loader: 'imgix', - }, - secondary: { - path: 'https://examplesecondary.com/images/', - loader: 'cloudinary', - }, - }, + path: 'https://example.com/myaccount/', + loader: 'imgix', }, } diff --git a/test/integration/image-component/basic/test/index.test.js b/test/integration/image-component/basic/test/index.test.js index ffa87e0c6b58f..a1aadbdc39f2c 100644 --- a/test/integration/image-component/basic/test/index.test.js +++ b/test/integration/image-component/basic/test/index.test.js @@ -40,11 +40,6 @@ function runTests() { await browser.elementById('preceding-slash-image').getAttribute('src') ).toBe('https://example.com/myaccount/fooslash.jpg') }) - it('should support manually selecting a different host', async () => { - expect( - await browser.elementById('secondary-image').getAttribute('src') - ).toBe('https://examplesecondary.com/images/foo2.jpg') - }) it('should add a srcset based on the loader', async () => { expect( await browser.elementById('basic-image').getAttribute('srcset') @@ -180,13 +175,6 @@ describe('Image Component Tests', () => { ) ).toBe(true) }) - it('should add a preload tag for a priority image, with secondary host', async () => { - expect( - await hasPreloadLinkMatchingUrl( - 'https://examplesecondary.com/images/withpriority2.png' - ) - ).toBe(true) - }) it('should add a preload tag for a priority image, with arbitrary host', async () => { expect( await hasPreloadLinkMatchingUrl( From 9c1c3b166f693cdb696ad39841a379c6a325e005 Mon Sep 17 00:00:00 2001 From: atcastle Date: Mon, 19 Oct 2020 21:42:49 -0700 Subject: [PATCH 3/4] Make image quality for loaders configurable with attributes --- packages/next/client/image.tsx | 85 ++++++++++++++----- .../image-component/basic/next.config.js | 1 + .../basic/pages/client-side.js | 2 +- .../image-component/basic/pages/index.js | 2 +- .../image-component/basic/test/index.test.js | 4 +- 5 files changed, 69 insertions(+), 25 deletions(-) diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index 95c84652faa28..1c68d75785442 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -16,6 +16,7 @@ type ImageProps = Omit< 'src' | 'srcSet' | 'ref' > & { src: string + quality?: string priority?: boolean lazy?: boolean unoptimized?: boolean @@ -23,6 +24,10 @@ type ImageProps = Omit< let imageData: any = process.env.__NEXT_IMAGE_OPTS const breakpoints = imageData.sizes || [640, 1024, 1600] +// Auto optimize defaults to on if not specified +if (imageData.autoOptimize === undefined) { + imageData.autoOptimize = true +} let cachedObserver: IntersectionObserver const IntersectionObserver = @@ -59,28 +64,39 @@ function getObserver(): IntersectionObserver | undefined { )) } -function computeSrc(src: string, unoptimized: boolean): string { +function computeSrc( + src: string, + unoptimized: boolean, + quality?: string +): string { if (unoptimized) { return src } - return callLoader(src) + return callLoader({ src, quality }) +} + +type CallLoaderProps = { + src: string + width?: number + quality?: string } -function callLoader(src: string, width?: number): string { +function callLoader(loaderProps: CallLoaderProps) { let loader = loaders[imageData.loader || 'default'] - return loader({ root: imageData.path, src, width }) + return loader({ root: imageData.path, ...loaderProps }) } type SrcSetData = { src: string widths: number[] + quality?: string } -function generateSrcSet({ src, widths }: SrcSetData): string { +function generateSrcSet({ src, widths, quality }: SrcSetData): string { // At each breakpoint, generate an image url using the loader, such as: // ' www.example.com/foo.jpg?w=480 480w, ' return widths - .map((width: number) => `${callLoader(src, width)} ${width}w`) + .map((width: number) => `${callLoader({ src, width, quality })} ${width}w`) .join(', ') } @@ -89,6 +105,7 @@ type PreloadData = { widths: number[] sizes?: string unoptimized?: boolean + quality?: string } function generatePreload({ @@ -96,6 +113,7 @@ function generatePreload({ widths, unoptimized = false, sizes, + quality, }: PreloadData): ReactElement { // This function generates an image preload that makes use of the "imagesrcset" and "imagesizes" // attributes for preloading responsive images. They're still experimental, but fully backward @@ -106,9 +124,9 @@ function generatePreload({ @@ -122,6 +140,7 @@ export default function Image({ priority = false, lazy = false, className, + quality, ...rest }: ImageProps) { const thisEl = useRef(null) @@ -159,11 +178,12 @@ export default function Image({ }, [thisEl, lazy]) // Generate attribute values - const imgSrc = computeSrc(src, unoptimized) + const imgSrc = computeSrc(src, unoptimized, quality) const imgSrcSet = !unoptimized ? generateSrcSet({ src, widths: breakpoints, + quality, }) : undefined @@ -220,23 +240,46 @@ export default function Image({ //BUILT IN LOADERS -type LoaderProps = { - root: string - src: string - width?: number -} +type LoaderProps = CallLoaderProps & { root: string } -function imgixLoader({ root, src, width }: LoaderProps): string { - return `${root}${src}${width ? '?w=' + width : ''}` +function imgixLoader({ root, src, width, quality }: LoaderProps): string { + const params = [] + let paramsString = '' + if (width) { + params.push('w=' + width) + } + if (quality) { + params.push('q=' + quality) + } + if (imageData.autoOptimize) { + params.push('auto=compress') + } + if (params.length) { + paramsString = '?' + params.join('&') + } + return `${root}${src}${paramsString}` } -function cloudinaryLoader({ root, src, width }: LoaderProps): string { - return `${root}${width ? 'w_' + width + '/' : ''}${src}` +function cloudinaryLoader({ root, src, width, quality }: LoaderProps): string { + const params = [] + let paramsString = '' + if (!quality && imageData.autoOptimize) { + quality = 'auto' + } + if (width) { + params.push('w_' + width) + } + if (quality) { + params.push('q_' + quality) + } + if (params.length) { + paramsString = params.join(',') + '/' + } + return `${root}${paramsString}${src}` } -function defaultLoader({ root, src, width }: LoaderProps): string { - // TODO: change quality parameter to be configurable +function defaultLoader({ root, src, width, quality }: LoaderProps): string { return `${root}?url=${encodeURIComponent(src)}&${ width ? `w=${width}&` : '' - }q=100` + }q=${quality || '100'}` } diff --git a/test/integration/image-component/basic/next.config.js b/test/integration/image-component/basic/next.config.js index 8f260dea043fd..45c3041575129 100644 --- a/test/integration/image-component/basic/next.config.js +++ b/test/integration/image-component/basic/next.config.js @@ -1,6 +1,7 @@ module.exports = { images: { sizes: [480, 1024, 1600], + autoOptimize: false, path: 'https://example.com/myaccount/', loader: 'imgix', }, diff --git a/test/integration/image-component/basic/pages/client-side.js b/test/integration/image-component/basic/pages/client-side.js index 0615e65d175aa..b4f7f9da18d5b 100644 --- a/test/integration/image-component/basic/pages/client-side.js +++ b/test/integration/image-component/basic/pages/client-side.js @@ -6,7 +6,7 @@ const ClientSide = () => { return (

This is a client side page

- + { return (

Hello World

- + { expect(await browser.elementById('basic-image').getAttribute('src')).toBe( - 'https://example.com/myaccount/foo.jpg' + 'https://example.com/myaccount/foo.jpg?q=60' ) }) it('should correctly generate src even if preceding slash is included in prop', async () => { @@ -44,7 +44,7 @@ function runTests() { expect( await browser.elementById('basic-image').getAttribute('srcset') ).toBe( - 'https://example.com/myaccount/foo.jpg?w=480 480w, https://example.com/myaccount/foo.jpg?w=1024 1024w, https://example.com/myaccount/foo.jpg?w=1600 1600w' + 'https://example.com/myaccount/foo.jpg?w=480&q=60 480w, https://example.com/myaccount/foo.jpg?w=1024&q=60 1024w, https://example.com/myaccount/foo.jpg?w=1600&q=60 1600w' ) }) it('should add a srcset even with preceding slash in prop', async () => { From f269565902122cd37830ac8daaace2dccb1811d4 Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Tue, 20 Oct 2020 16:27:18 +0200 Subject: [PATCH 4/4] Remove unneeded tests --- .../bad-next-config/pages/index.js | 7 -- .../bad-next-config/test/index.test.js | 76 ------------------- 2 files changed, 83 deletions(-) delete mode 100644 test/integration/image-component/bad-next-config/pages/index.js delete mode 100644 test/integration/image-component/bad-next-config/test/index.test.js diff --git a/test/integration/image-component/bad-next-config/pages/index.js b/test/integration/image-component/bad-next-config/pages/index.js deleted file mode 100644 index ff2547f9c51e9..0000000000000 --- a/test/integration/image-component/bad-next-config/pages/index.js +++ /dev/null @@ -1,7 +0,0 @@ -import React from 'react' - -const page = () => { - return
Hello
-} - -export default page diff --git a/test/integration/image-component/bad-next-config/test/index.test.js b/test/integration/image-component/bad-next-config/test/index.test.js deleted file mode 100644 index da8c8963c6fbd..0000000000000 --- a/test/integration/image-component/bad-next-config/test/index.test.js +++ /dev/null @@ -1,76 +0,0 @@ -/* eslint-env jest */ - -import { join } from 'path' -import { nextBuild } from 'next-test-utils' -import fs from 'fs-extra' - -jest.setTimeout(1000 * 30) - -const appDir = join(__dirname, '../') -const nextConfig = join(appDir, 'next.config.js') - -describe('Next.config.js images prop without default host', () => { - let build - beforeAll(async () => { - await fs.writeFile( - nextConfig, - `module.exports = { - images: { - sizes: [480, 1024, 1600], - hosts: { - secondary: { - path: 'https://examplesecondary.com/images/', - loader: 'cloudinary', - }, - }, - }, - }`, - 'utf8' - ) - build = await nextBuild(appDir, [], { - stdout: true, - stderr: true, - }) - }) - it('Should error during build if images prop in next.config is malformed', () => { - expect(build.stderr).toContain( - 'If the image configuration property is present in next.config.js, it must have a host named "default"' - ) - }) -}) - -describe('Next.config.js images prop without path', () => { - let build - beforeAll(async () => { - await fs.writeFile( - nextConfig, - `module.exports = { - images: { - sizes: [480, 1024, 1600], - hosts: { - default: { - path: 'https://examplesecondary.com/images/', - loader: 'cloudinary', - }, - secondary: { - loader: 'cloudinary', - }, - }, - }, - }`, - 'utf8' - ) - build = await nextBuild(appDir, [], { - stdout: true, - stderr: true, - }) - }) - afterAll(async () => { - await fs.remove(nextConfig) - }) - it('Should error during build if images prop in next.config is malformed', () => { - expect(build.stderr).toContain( - 'All hosts defined in the image configuration property of next.config.js must define a path' - ) - }) -})