Skip to content

Commit

Permalink
Fix Image srcset to ensure the component's width is the largest p…
Browse files Browse the repository at this point in the history
…ossible image (#18236)

There was a bug when the user defines a width on the Image component, but a larger size image is requested.

This is because the browser uses the `srcset` to decide which image size to request and we had the srcset basically hardcoded.

This PR fixes the behavior so that the `srcset` will never be larger than the `width` defined on the component.

It also fixes a bug where the preload srcset did not match the img srcset.

- Related to #18147 
- Related to #18122
  • Loading branch information
styfle committed Oct 26, 2020
1 parent 9c65c99 commit 61fab92
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 81 deletions.
154 changes: 98 additions & 56 deletions packages/next/client/image.tsx
Expand Up @@ -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 =
Expand Down Expand Up @@ -80,15 +79,32 @@ 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,
width: number | undefined,
quality?: string
): string {
if (unoptimized) {
return src
}
return callLoader({ src, width: largestSize, quality })
const widths = getWidthsFromConfig(width)
const largest = widths[widths.length - 1]
return callLoader({ src, width: largest, quality })
}

type CallLoaderProps = {
Expand All @@ -104,29 +120,38 @@ 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, '
return widths
.map((width: number) => `${callLoader({ src, width, quality })} ${width}w`)
if (unoptimized) {
return undefined
}
return getWidthsFromConfig(width)
.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,
Expand All @@ -140,15 +165,25 @@ function generatePreload({
<link
rel="preload"
as="image"
href={computeSrc(src, unoptimized, quality)}
href={computeSrc(src, unoptimized, width, 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}
/>
</Head>
)
}

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,
Expand All @@ -165,6 +200,13 @@ export default function Image({
const thisEl = useRef<HTMLImageElement>(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(
Expand Down Expand Up @@ -200,58 +242,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
) {
// <Image src="i.png" width={100} height={100} />
// <Image src="i.png" width="100" height="100" />
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',
Expand All @@ -266,8 +273,8 @@ export default function Image({
width: '100%',
}
} else if (
typeof height === 'undefined' &&
typeof width === 'undefined' &&
typeof widthInt === 'undefined' &&
typeof heightInt === 'undefined' &&
unsized
) {
// <Image src="i.png" unsized />
Expand All @@ -288,6 +295,41 @@ export default function Image({
}
}

// Generate attribute values
const imgSrc = computeSrc(src, unoptimized, widthInt, 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'
Expand All @@ -298,7 +340,7 @@ export default function Image({
{shouldPreload
? generatePreload({
src,
widths: configSizes,
width: widthInt,
unoptimized,
sizes,
quality,
Expand Down
2 changes: 1 addition & 1 deletion test/integration/image-component/basic/next.config.js
@@ -1,6 +1,6 @@
module.exports = {
images: {
sizes: [480, 1024, 1600],
sizes: [480, 1024, 1600, 2000],
path: 'https://example.com/myaccount/',
loader: 'imgix',
},
Expand Down
3 changes: 1 addition & 2 deletions test/integration/image-component/basic/pages/index.js
Expand Up @@ -19,7 +19,7 @@ const Page = () => {
data-demo="demo-value"
src="bar.jpg"
loading="eager"
width={300}
width={1024}
height={400}
/>
<Image
Expand All @@ -39,7 +39,6 @@ const Page = () => {
width={300}
height={400}
/>
<Image id="priority-image" priority src="withpriority.png" />
<Image
id="priority-image"
priority
Expand Down
6 changes: 3 additions & 3 deletions test/integration/image-component/basic/pages/lazy.js
Expand Up @@ -9,7 +9,7 @@ const Lazy = () => {
id="lazy-top"
src="foo1.jpg"
height={400}
width={300}
width={1024}
loading="lazy"
></Image>
<div style={{ height: '2000px' }}></div>
Expand All @@ -35,15 +35,15 @@ const Lazy = () => {
id="lazy-without-attribute"
src="foo4.jpg"
height={400}
width={300}
width={800}
></Image>
<div style={{ height: '2000px' }}></div>
<Image
id="eager-loading"
src="foo5.jpg"
loading="eager"
height={400}
width={300}
width={1900}
></Image>
</div>
)
Expand Down

0 comments on commit 61fab92

Please sign in to comment.