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

Fix error when next/future/image is missing width or height #38132

Merged
merged 8 commits into from
Jun 29, 2022
26 changes: 18 additions & 8 deletions packages/next/client/future/image.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,13 @@ function generateImgAttrs({
}

function getInt(x: unknown): number | undefined {
if (typeof x === 'number') {
if (typeof x === 'number' || typeof x === 'undefined') {
return x
}
if (typeof x === 'string') {
if (typeof x === 'string' && /^[0-9]+$/.test(x)) {
return parseInt(x, 10)
}
return undefined
return NaN
}

// See https://stackoverflow.com/q/39777833/266535 for why we use this ref
Expand Down Expand Up @@ -382,12 +382,22 @@ export default function Image({
)}`
)
}
if (
(typeof widthInt !== 'undefined' && isNaN(widthInt)) ||
(typeof heightInt !== 'undefined' && isNaN(heightInt))
) {
if (typeof widthInt === 'undefined') {
throw new Error(
`Image with src "${src}" is missing required "width" property.`
)
} else if (isNaN(widthInt)) {
throw new Error(
`Image with src "${src}" has invalid "width" property. Expected a numeric value in pixels but received "${width}".`
)
}
if (typeof heightInt === 'undefined') {
throw new Error(
`Image with src "${src}" is missing required "height" property.`
)
} else if (isNaN(heightInt)) {
throw new Error(
`Image with src "${src}" has invalid "width" or "height" property. These should be numeric values.`
`Image with src "${src}" has invalid "height" property. Expected a numeric value in pixels but received "${height}".`
)
}
if (!VALID_LOADING_VALUES.includes(loading)) {
Expand Down
12 changes: 12 additions & 0 deletions test/integration/image-future/default/pages/invalid-height.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react'
import Image from 'next/future/image'

export default function Page() {
return (
<div>
<p>Invalid height</p>

<Image src="/test.jpg" width={400} height="50vh" />
</div>
)
}

This file was deleted.

12 changes: 12 additions & 0 deletions test/integration/image-future/default/pages/invalid-width.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react'
import Image from 'next/future/image'

export default function Page() {
return (
<div>
<p>Invalid width</p>

<Image src="/test.jpg" width="100%" height={300} />
</div>
)
}
11 changes: 11 additions & 0 deletions test/integration/image-future/default/pages/missing-height.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import React from 'react'
import Image from 'next/future/image'

export default function Page() {
return (
<div>
<h1>Missing height</h1>
<Image src="/test.jpg" width="100" />
</div>
)
}
11 changes: 11 additions & 0 deletions test/integration/image-future/default/pages/missing-width.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import React from 'react'
import Image from 'next/future/image'

export default function Page() {
return (
<div>
<h1>Missing width or height</h1>
<Image src="/test.jpg" height={100} />
</div>
)
}
2 changes: 2 additions & 0 deletions test/integration/image-future/default/pages/on-load.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ const Page = () => {
id="2"
src={clicked ? require('../public/test.png') : red}
placeholder={clicked ? 'blur' : 'empty'}
width="256"
height="256"
/>

<ImageWithMessage
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ const Page = () => {
<ImageWithMessage
id="6"
src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAMAAAAFCAYAAACAcVaiAAAAE0lEQVR42mNk+P+/ngEKGMngAADDdwx3Uz/3AAAAAABJRU5ErkJggg=="
width={128}
height={128}
idToCount={idToCount}
setIdToCount={setIdToCount}
/>
Expand Down Expand Up @@ -108,9 +110,9 @@ function ImageWithMessage({ id, idToCount, setIdToCount, ...props }) {
count++
idToCount[id] = count
setIdToCount(idToCount)
const msg = `loaded ${count} img${id} with dimensions ${naturalWidth}x${naturalHeight}`
setMsg(msg)
console.log(msg)
setMsg(
`loaded ${count} img${id} with dimensions ${naturalWidth}x${naturalHeight}`
)
}}
{...props}
/>
Expand Down
206 changes: 113 additions & 93 deletions test/integration/image-future/default/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,98 +206,91 @@ function runTests(mode) {
})

it('should callback onLoadingComplete when image is fully loaded', async () => {
let browser
try {
browser = await webdriver(appPort, '/on-loading-complete')
let browser = await webdriver(appPort, '/on-loading-complete')

await browser.eval(
`document.getElementById("footer").scrollIntoView({behavior: "smooth"})`
)
await browser.eval(
`document.getElementById("footer").scrollIntoView({behavior: "smooth"})`
)

await check(
() => browser.eval(`document.getElementById("img1").currentSrc`),
/test(.*)jpg/
)
await check(
() => browser.eval(`document.getElementById("img2").currentSrc`),
/test(.*).png/
)
await check(
() => browser.eval(`document.getElementById("img3").currentSrc`),
/test\.svg/
)
await check(
() => browser.eval(`document.getElementById("img4").currentSrc`),
/test(.*)ico/
)
await check(
() => browser.eval(`document.getElementById("msg1").textContent`),
'loaded 1 img1 with dimensions 128x128'
)
await check(
() => browser.eval(`document.getElementById("msg2").textContent`),
'loaded 1 img2 with dimensions 400x400'
)
await check(
() => browser.eval(`document.getElementById("msg3").textContent`),
'loaded 1 img3 with dimensions 400x400'
)
await check(
() => browser.eval(`document.getElementById("msg4").textContent`),
'loaded 1 img4 with dimensions 32x32'
)
await check(
() => browser.eval(`document.getElementById("msg5").textContent`),
'loaded 1 img5 with dimensions 3x5'
)
await check(
() => browser.eval(`document.getElementById("msg6").textContent`),
'loaded 1 img6 with dimensions 3x5'
)
await check(
() => browser.eval(`document.getElementById("msg7").textContent`),
'loaded 1 img7 with dimensions 400x400'
)
await check(
() => browser.eval(`document.getElementById("msg8").textContent`),
'loaded 1 img8 with dimensions 640x373'
)
await check(
() =>
browser.eval(
`document.getElementById("img8").getAttribute("data-nimg")`
),
'future'
)
await check(
() => browser.eval(`document.getElementById("img8").currentSrc`),
/wide.png/
)
await browser.eval('document.getElementById("toggle").click()')
await check(
() => browser.eval(`document.getElementById("msg8").textContent`),
'loaded 2 img8 with dimensions 400x300'
)
await check(
() =>
browser.eval(
`document.getElementById("img8").getAttribute("data-nimg")`
),
'future'
)
await check(
() => browser.eval(`document.getElementById("img8").currentSrc`),
/test-rect.jpg/
)
await check(
() => browser.eval(`document.getElementById("msg9").textContent`),
'loaded 1 img9 with dimensions 400x400'
)
} finally {
if (browser) {
await browser.close()
}
}
await check(
() => browser.eval(`document.getElementById("img1").currentSrc`),
/test(.*)jpg/
)
await check(
() => browser.eval(`document.getElementById("img2").currentSrc`),
/test(.*).png/
)
await check(
() => browser.eval(`document.getElementById("img3").currentSrc`),
/test\.svg/
)
await check(
() => browser.eval(`document.getElementById("img4").currentSrc`),
/test(.*)ico/
)
await check(
() => browser.eval(`document.getElementById("msg1").textContent`),
'loaded 1 img1 with dimensions 128x128'
)
await check(
() => browser.eval(`document.getElementById("msg2").textContent`),
'loaded 1 img2 with dimensions 400x400'
)
await check(
() => browser.eval(`document.getElementById("msg3").textContent`),
'loaded 1 img3 with dimensions 400x400'
)
await check(
() => browser.eval(`document.getElementById("msg4").textContent`),
'loaded 1 img4 with dimensions 32x32'
)
await check(
() => browser.eval(`document.getElementById("msg5").textContent`),
'loaded 1 img5 with dimensions 3x5'
)
await check(
() => browser.eval(`document.getElementById("msg6").textContent`),
'loaded 1 img6 with dimensions 3x5'
)
await check(
() => browser.eval(`document.getElementById("msg7").textContent`),
'loaded 1 img7 with dimensions 400x400'
)
await check(
() => browser.eval(`document.getElementById("msg8").textContent`),
'loaded 1 img8 with dimensions 640x373'
)
await check(
() =>
browser.eval(
`document.getElementById("img8").getAttribute("data-nimg")`
),
'future'
)
await check(
() => browser.eval(`document.getElementById("img8").currentSrc`),
/wide.png/
)
await browser.eval('document.getElementById("toggle").click()')
await check(
() => browser.eval(`document.getElementById("msg8").textContent`),
'loaded 2 img8 with dimensions 400x300'
)
await check(
() =>
browser.eval(
`document.getElementById("img8").getAttribute("data-nimg")`
),
'future'
)
await check(
() => browser.eval(`document.getElementById("img8").currentSrc`),
/test-rect.jpg/
)
await check(
() => browser.eval(`document.getElementById("msg9").textContent`),
'loaded 1 img9 with dimensions 400x400'
)
})

it('should callback native onLoad in most cases', async () => {
Expand Down Expand Up @@ -645,12 +638,39 @@ function runTests(mode) {
)
})

it('should show error when not numeric string width or height', async () => {
const browser = await webdriver(appPort, '/invalid-width-or-height')
it('should show error when invalid width prop', async () => {
const browser = await webdriver(appPort, '/invalid-width')

expect(await hasRedbox(browser)).toBe(true)
expect(await getRedboxHeader(browser)).toContain(
`Image with src "/test.jpg" has invalid "width" property. Expected a numeric value in pixels but received "100%".`
)
})

it('should show error when invalid height prop', async () => {
const browser = await webdriver(appPort, '/invalid-height')

expect(await hasRedbox(browser)).toBe(true)
expect(await getRedboxHeader(browser)).toContain(
`Image with src "/test.jpg" has invalid "height" property. Expected a numeric value in pixels but received "50vh".`
)
})

it('should show error when missing width prop', async () => {
const browser = await webdriver(appPort, '/missing-width')

expect(await hasRedbox(browser)).toBe(true)
expect(await getRedboxHeader(browser)).toContain(
`Image with src "/test.jpg" is missing required "width" property.`
)
})

it('should show error when missing height prop', async () => {
const browser = await webdriver(appPort, '/missing-height')

expect(await hasRedbox(browser)).toBe(true)
expect(await getRedboxHeader(browser)).toContain(
`Image with src "/test.jpg" has invalid "width" or "height" property. These should be numeric values.`
`Image with src "/test.jpg" is missing required "height" property.`
)
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ type ImageCardProps = ImageProps & {
* Example of using the `Image` component in a HOC.
*/
export function ImageCard(props: ImageCardProps) {
return <Image {...props} layout="fill" />
return <Image {...props} width="400" height="400" />
}
2 changes: 1 addition & 1 deletion test/integration/image-future/typescript/next.config.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module.exports = {
images: {
domains: ['via.placeholder.com'],
// disableStaticImages: true,
disableStaticImages: true,
Copy link
Member

Choose a reason for hiding this comment

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

Was this change intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, fixed!

(must have been changed by running tests locally)

styfle marked this conversation as resolved.
Show resolved Hide resolved
},
experimental: {
images: {
Expand Down