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

This PR fixes the case when `next/future/image` is missing the width or height props. It also fixes a case when `100%` or `100vh` were incorrectly parsed as `100` pixels.
vercel · Jun 29, 2022 · 4df0778 · 4df0778
1 parent 876d4d1
commit 4df0778
Show file tree

Hide file tree

Showing 10 changed files with 185 additions and 123 deletions.
diff --git a/packages/next/client/future/image.tsx b/packages/next/client/future/image.tsx
@@ -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
@@ -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)) {

diff --git a/test/integration/image-future/default/pages/invalid-height.js b/test/integration/image-future/default/pages/invalid-height.js
@@ -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>
+  )
+}
diff --git a/test/integration/image-future/default/pages/invalid-width-or-height.js b/test/integration/image-future/default/pages/invalid-width-or-height.js
diff --git a/test/integration/image-future/default/pages/invalid-width.js b/test/integration/image-future/default/pages/invalid-width.js
@@ -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>
+  )
+}
diff --git a/test/integration/image-future/default/pages/missing-height.js b/test/integration/image-future/default/pages/missing-height.js
@@ -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>
+  )
+}
diff --git a/test/integration/image-future/default/pages/missing-width.js b/test/integration/image-future/default/pages/missing-width.js
@@ -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>
+  )
+}
diff --git a/test/integration/image-future/default/pages/on-load.js b/test/integration/image-future/default/pages/on-load.js
@@ -29,6 +29,8 @@ const Page = () => {
         id="2"
         src={clicked ? require('../public/test.png') : red}
         placeholder={clicked ? 'blur' : 'empty'}
+        width="256"
+        height="256"
       />
 
       <ImageWithMessage

diff --git a/test/integration/image-future/default/pages/on-loading-complete.js b/test/integration/image-future/default/pages/on-loading-complete.js
@@ -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}
       />
@@ -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}
         />

diff --git a/test/integration/image-future/default/test/index.test.js b/test/integration/image-future/default/test/index.test.js
@@ -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 () => {
@@ -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.`
       )
     })
 

diff --git a/test/integration/image-future/typescript/components/image-card.tsx b/test/integration/image-future/typescript/components/image-card.tsx
@@ -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" />
 }