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.

Sign up for GitHub

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

Jump to bottom

fix: skip resizing image if it's animated #39325

Merged
merged 7 commits into from Aug 5, 2022
Merged

fix: skip resizing image if it's animated #39325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d23f79a
add `is-animated` types
balazsorban44 Aug 4, 2022
8a730f7
add test
balazsorban44 Aug 4, 2022
97dac8d
fix: skip resizing image if it's animated
balazsorban44 Aug 4, 2022
d456e3e
remove tests
balazsorban44 Aug 5, 2022
56c93f3
update tests
balazsorban44 Aug 5, 2022
f5e4249
Merge branch 'canary' into fix/issue-39317
balazsorban44 Aug 5, 2022
9056b80
Merge branch 'canary' into fix/issue-39317
kodiakhq[bot] Aug 5, 2022
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
5 changes: 3 additions & 2 deletions packages/next/server/image-optimizer.ts
View file
Open in desktop
Expand Up @@ -4,7 +4,6 @@ import { promises } from 'fs'
import { getOrientation, Orientation } from 'next/dist/compiled/get-orientation'
import imageSizeOf from 'next/dist/compiled/image-size'
import { IncomingMessage, ServerResponse } from 'http'
// @ts-ignore no types for is-animated
import isAnimated from 'next/dist/compiled/is-animated'
import contentDisposition from 'next/dist/compiled/content-disposition'
import { join } from 'path'
Expand Down Expand Up @@ -726,7 +725,9 @@ export async function resizeImage(
extension: 'avif' | 'webp' | 'png' | 'jpeg',
quality: number
): Promise<Buffer> {
if (sharp) {
if (isAnimated(content)) {
return content
} else if (sharp) {
const transformer = sharp(content)

if (extension === 'avif') {
Expand Down
4 changes: 4 additions & 0 deletions packages/next/types/misc.d.ts
View file
Open in desktop
Expand Up @@ -367,3 +367,7 @@ declare module 'next/dist/compiled/watchpack' {

export default Watchpack
}

declare module 'next/dist/compiled/is-animated' {
export default function isAnimated(buffer: Buffer): boolean
}
37 changes: 32 additions & 5 deletions test/integration/image-optimizer/app/pages/index.js
View file
Open in desktop
@@ -1,13 +1,40 @@
import Image from 'next/image'
import Logo from '../public/test.jpg'

function Home() {
import img1 from '../public/animated2.png'
import img2 from '../public/äöüščří.png'
import img3 from '../public/test.avif'
import img4 from '../public/test.jpg'
import img5 from '../public/test.webp'
import img6 from '../public/animated.gif'
import img7 from '../public/grayscale.png'
import img8 from '../public/test.bmp'
import img9 from '../public/test.png'
import img11 from '../public/animated.png'
import img12 from '../public/mountains.jpg'
import img13 from '../public/test.gif'
import img14 from '../public/test.svg'
import img15 from '../public/animated.webp'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: #39325 (review)

The issue was that we only imported a single image file on the page, so the nextBuild command did not fail early. I added all the valid image imports.

This line should now cover the fix in this PR.

We might want to create a separate test suite to check if Next.js builds with valid/invalid images and gives a proper message if it's the latter.

E.g.: I noticed that importing .tiff does not work. The build throws an error that there is no appropriate webpack loader for this file. I thought of adding it here:

next.js/packages/next/build/webpack-config.ts

Line 233 in d88fd14

/\.(png|jpg|jpeg|gif|webp|avif|ico|bmp|svg)$/i

But then I get this build error:

/public/test.tiff
TypeError: Tiff doesn't support buffer
    at Object.calculate (.../next.js/packages/next/dist/compiled/image-size/index.js:1:12871)

So I am actually not sure about the level of our TIFF support here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support TIFF

Copy link
Member

@styfle styfle Aug 5, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update the PR description with this info to mention it was only affecting statically imported images during next build

import img17 from '../public/test.ico'

export default function Home() {
return (
<>
<h1>Image Optimizer Home</h1>
<Image src={Logo} />
<Image src={img1} />
<Image src={img2} />
<Image src={img3} />
<Image src={img4} />
<Image src={img5} />
<Image src={img6} />
<Image src={img7} />
<Image src={img8} />
<Image src={img9} />
<Image src={img11} />
<Image src={img12} />
<Image src={img13} />
<Image src={img14} />
<Image src={img15} />
<Image src={img17} />
</>
)
}

export default Home
2 changes: 1 addition & 1 deletion ...ration/image-optimizer/test/index.test.js → ...ration/image-optimizer/test/index.test.ts
View file
Open in desktop
Expand Up @@ -374,7 +374,7 @@ describe('Image Optimizer', () => {
describe('Server support for minimumCacheTTL in next.config.js', () => {
const size = 96 // defaults defined in server/config.ts
const dangerouslyAllowSVG = true
const ctx = {
const ctx: any = {
w: size,
isDev: false,
domains,
Expand Down
0 ...on/image-optimizer/test/old-sharp.test.js → ...on/image-optimizer/test/old-sharp.test.ts
View file
Open in desktop
File renamed without changes.
0 ...ration/image-optimizer/test/sharp.test.js → ...ration/image-optimizer/test/sharp.test.ts
View file
Open in desktop
File renamed without changes.
0 ...tion/image-optimizer/test/squoosh.test.js → ...tion/image-optimizer/test/squoosh.test.ts
View file
Open in desktop
File renamed without changes.
17 changes: 7 additions & 10 deletions .../integration/image-optimizer/test/util.js → .../integration/image-optimizer/test/util.ts
View file
Open in desktop
Expand Up @@ -40,11 +40,8 @@ export async function serveSlowImage() {
res.end(await fs.readFile(join(__dirname, '../app/public/test.png')))
})

await new Promise((resolve, reject) => {
server.listen(port, (err) => {
if (err) return reject(err)
resolve()
})
styfle marked this conversation as resolved.
Show resolved Hide resolved
await new Promise((resolve) => {
server.listen(port, () => resolve(true))
})
console.log(`Started slow image server at ::${port}`)
return {
Expand Down Expand Up @@ -129,7 +126,7 @@ async function fetchWithDuration(...args) {

export function runTests(ctx) {
const { isDev, minimumCacheTTL = 60 } = ctx
let slowImageServer
let slowImageServer: Awaited<ReturnType<typeof serveSlowImage>>
beforeAll(async () => {
slowImageServer = await serveSlowImage()
})
Expand Down Expand Up @@ -1248,7 +1245,7 @@ export const setupTests = (ctx) => {
await cleanImagesDir(ctx)
})
afterAll(async () => {
await killApp(curCtx.app)
if (curCtx.app) await killApp(curCtx.app)
})

runTests(curCtx)
Expand Down Expand Up @@ -1288,8 +1285,8 @@ export const setupTests = (ctx) => {
})
})
afterAll(async () => {
await killApp(curCtx.app)
nextConfig.restore()
if (curCtx.app) await killApp(curCtx.app)
})

runTests(curCtx)
Expand Down Expand Up @@ -1321,7 +1318,7 @@ export const setupTests = (ctx) => {
})
})
afterAll(async () => {
await killApp(curCtx.app)
if (curCtx.app) await killApp(curCtx.app)
})

runTests(curCtx)
Expand Down Expand Up @@ -1362,8 +1359,8 @@ export const setupTests = (ctx) => {
})
})
afterAll(async () => {
await killApp(curCtx.app)
nextConfig.restore()
if (curCtx.app) await killApp(curCtx.app)
})

runTests(curCtx)
Expand Down