Skip to content
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

Change Image component lazy=true to loading=lazy #18138

Merged
merged 7 commits into from Oct 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 18 additions & 9 deletions packages/next/client/image.tsx
@@ -1,6 +1,9 @@
import React, { ReactElement, useEffect, useRef } from 'react'
import Head from '../next-server/lib/head'

const VALID_LOADING_VALUES = ['lazy', 'eager', undefined] as const
type LoadingValue = typeof VALID_LOADING_VALUES[number]

const loaders = new Map<LoaderKey, (props: LoaderProps) => string>([
['imgix', imgixLoader],
['cloudinary', cloudinaryLoader],
Expand All @@ -18,12 +21,12 @@ type ImageData = {

type ImageProps = Omit<
JSX.IntrinsicElements['img'],
'src' | 'srcSet' | 'ref' | 'width' | 'height'
'src' | 'srcSet' | 'ref' | 'width' | 'height' | 'loading'
> & {
src: string
quality?: string
priority?: boolean
lazy?: boolean
loading?: LoadingValue
unoptimized?: boolean
} & (
| { width: number; height: number; unsized?: false }
Expand Down Expand Up @@ -142,7 +145,7 @@ export default function Image({
sizes,
unoptimized = false,
priority = false,
lazy,
loading,
className,
quality,
width,
Expand All @@ -152,17 +155,23 @@ export default function Image({
}: ImageProps) {
const thisEl = useRef<HTMLImageElement>(null)

// Sanity Checks:
// If priority and lazy are present, log an error and use priority only.
if (priority && lazy) {
if (process.env.NODE_ENV !== 'production') {
if (process.env.NODE_ENV !== 'production') {
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(
String
).join(',')}.`
)
}
if (priority && loading === 'lazy') {
throw new Error(
`Image with src "${src}" has both "priority" and "lazy" properties. Only one should be used.`
`Image with src "${src}" has both "priority" and "loading=lazy" properties. Only one should be used.`
)
}
}

if (!priority && typeof lazy === 'undefined') {
let lazy = loading === 'lazy'
if (!priority && typeof loading === 'undefined') {
lazy = true
}

Expand Down
8 changes: 4 additions & 4 deletions test/integration/image-component/basic/pages/client-side.js
Expand Up @@ -9,7 +9,7 @@ const ClientSide = () => {
<Image
id="basic-image"
src="foo.jpg"
lazy={false}
loading="eager"
width={300}
height={400}
quality={60}
Expand All @@ -18,7 +18,7 @@ const ClientSide = () => {
id="attribute-test"
data-demo="demo-value"
src="bar.jpg"
lazy={false}
loading="eager"
width={300}
height={400}
/>
Expand All @@ -27,15 +27,15 @@ const ClientSide = () => {
data-demo="demo-value"
host="secondary"
src="foo2.jpg"
lazy={false}
loading="eager"
width={300}
height={400}
/>
<Image
id="unoptimized-image"
unoptimized
src="https://arbitraryurl.com/foo.jpg"
lazy={false}
loading="eager"
width={300}
height={400}
/>
Expand Down
8 changes: 4 additions & 4 deletions test/integration/image-component/basic/pages/index.js
Expand Up @@ -9,7 +9,7 @@ const Page = () => {
<Image
id="basic-image"
src="foo.jpg"
lazy={false}
loading="eager"
width={300}
height={400}
quality={60}
Expand All @@ -18,7 +18,7 @@ const Page = () => {
id="attribute-test"
data-demo="demo-value"
src="bar.jpg"
lazy={false}
loading="eager"
width={300}
height={400}
/>
Expand All @@ -27,15 +27,15 @@ const Page = () => {
data-demo="demo-value"
host="secondary"
src="foo2.jpg"
lazy={false}
loading="eager"
width={300}
height={400}
/>
<Image
id="unoptimized-image"
unoptimized
src="https://arbitraryurl.com/foo.jpg"
lazy={false}
loading="eager"
width={300}
height={400}
/>
Expand Down
27 changes: 24 additions & 3 deletions test/integration/image-component/basic/pages/lazy.js
Expand Up @@ -5,12 +5,18 @@ const Lazy = () => {
return (
<div>
<p id="stubtext">This is a page with lazy-loaded images</p>
<Image id="lazy-top" src="foo1.jpg" height={400} width={300} lazy></Image>
<Image
id="lazy-top"
src="foo1.jpg"
height={400}
width={300}
loading="lazy"
></Image>
<div style={{ height: '2000px' }}></div>
<Image
id="lazy-mid"
src="foo2.jpg"
lazy
loading="lazy"
height={400}
width={300}
className="exampleclass"
Expand All @@ -22,7 +28,22 @@ const Lazy = () => {
height={400}
width={300}
unoptimized
lazy
loading="lazy"
></Image>
<div style={{ height: '2000px' }}></div>
<Image
id="lazy-without-attribute"
src="foo4.jpg"
height={400}
width={300}
></Image>
<div style={{ height: '2000px' }}></div>
<Image
id="eager-loading"
src="foo5.jpg"
loading="eager"
height={400}
width={300}
></Image>
</div>
)
Expand Down
32 changes: 32 additions & 0 deletions test/integration/image-component/basic/test/index.test.js
Expand Up @@ -132,6 +132,38 @@ function lazyLoadingTests() {
await browser.elementById('lazy-bottom').getAttribute('srcset')
).toBeFalsy()
})
it('should load the fourth image lazily after scrolling down', async () => {
expect(
await browser.elementById('lazy-without-attribute').getAttribute('src')
).toBeFalsy()
expect(
await browser.elementById('lazy-without-attribute').getAttribute('srcset')
).toBeFalsy()
let viewportHeight = await browser.eval(`window.innerHeight`)
let topOfBottomImage = await browser.eval(
`document.getElementById('lazy-without-attribute').parentElement.offsetTop`
)
let buffer = 150
await browser.eval(
`window.scrollTo(0, ${topOfBottomImage - (viewportHeight + buffer)})`
)
await waitFor(200)
expect(
await browser.elementById('lazy-without-attribute').getAttribute('src')
).toBe('https://example.com/myaccount/foo4.jpg?auto=format')
expect(
await browser.elementById('lazy-without-attribute').getAttribute('srcset')
).toBeTruthy()
})

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'
)
expect(
await browser.elementById('eager-loading').getAttribute('srcset')
).toBeTruthy()
})
}

async function hasPreloadLinkMatchingUrl(url) {
Expand Down