Fix Show error when user put wrong values in width or height #26166

jthcast · 2021-06-16T06:17:31Z

Bug

Related issues linked using fixes #26135
Integration tests added

fixes #26135

Feature

Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
Related issues linked using fixes #26135
Integration tests added
Documentation added
Telemetry added. In case of a feature if it's used or not.

Documentation / Examples

Make sure the linting passes

timneutkens

Can you add a integration test to test/integration/image-component

atcastle

Thanks, this is an error case that was missing. With an integration test, this looks good to me.

jthcast · 2021-06-16T21:41:25Z

a... am I doing right? I'm new to this test 😅

timneutkens · 2021-06-17T08:55:47Z

packages/next/client/image.tsx

@@ -353,6 +353,9 @@ export default function Image({
        )}`
      )
    }
+    if (Number.isNaN(widthInt) || Number.isNaN(heightInt)){


width and height are not required when layout="fill' right? That seems like it would throw here

layout need fixed value

fill X

responsive O

intrinsic O

fixed O

You are right. it only can handle case that not layout='fill'.
code should change like below

Suggested change

if (Number.isNaN(widthInt) || Number.isNaN(heightInt)){

if (layout !== 'fill' && (Number.isNaN(widthInt) || Number.isNaN(heightInt)){

When layout='fill' it doesn't need width and height. so don't know that value.
therefore, the following additional process is required

if (layout !== 'fill' && ((widthInt || 0) * (heightInt || 0) < 1600)) { console.warn(`Image with src "${src}" is smaller than 40x40. Consider removing the "placeholder='blur'" property to improve performance.`); }

But there is another problem that when layout='fill' and pixel size is lower then 40x40.

I tried get real width value from image but failed..

Are we sure this would have thrown on an empty image with layout fill? I believe widthInt is undefined if nothing is provided, and Number.isNaN(undefined) is false

No. as i said, this PR can't handle when layout='fill'. when layout='fill' it doesn't need width and height. and don't know it's real size.

And, as you said Number.isNaN(undefined) return false. but i changed it to isNaN.
isNaN(undefined) return true.

test/integration/image-component/default/pages/invalid-weight-or-height.js

packages/next/client/image.tsx

…or-height.js Co-authored-by: Steven <steven@ceriously.com>

Co-authored-by: Steven <steven@ceriously.com>

test/integration/image-component/default/test/index.test.js

Co-authored-by: Steven <steven@ceriously.com>

test/integration/image-component/default/pages/invalid-placeholder-blur-static.js

ijjk · 2021-06-17T18:52:08Z

Failing test suites

Commit: 168597d

test/integration/image-component/typescript/test/index.test.js

TypeScript Image Component > next dev > should print error when invalid Image usage

Expand output

● TypeScript Image Component › next dev › should print error when invalid Image usage

expect(received).toMatch(expected)

Expected pattern: /must use "width" and "height" properties or "layout='fill'" property/
Received string:  "ready - started server on 0.0.0.0:40719, url: http://localhost:40719
info  - Using webpack 5. Reason: Enabled by default https://nextjs.org/docs/messages/webpack5
warn  - Minimum recommended TypeScript version is v4.3.2, older versions can potentially be incompatible with Next.js. Detected: 4.3.0-beta
event - compiled successfully
event - build page: /valid
wait  - compiling...
event - compiled successfully
event - build page: /invalid
wait  - compiling...
event - compiled successfully
event - build page: /next/dist/pages/_error
wait  - compiling...
event - compiled successfully
Error: Image with src \"https://via.placeholder.com/500\" has invalid \"width\" or \"height\" property. These should be numeric values.

  345 |
  346 |     if (layout !== 'fill' && (isNaN(widthInt) || isNaN(heightInt))) {
> 347 |       throw new Error(`Image with src "${src}" has invalid "width" or "height" property. These should be numeric values.`);
      |             ^
  348 |     }
  349 |
  350 |     if (!VALID_LOADING_VALUES.includes(loading)) {

  at Image (integration/image-component/typescript/.next/server/pages/invalid.js:347:13)
  at processChild (../node_modules/react-dom/cjs/react-dom-server.node.development.js:3353:14)
  at resolve (../node_modules/react-dom/cjs/react-dom-server.node.development.js:3270:5)
  at ReactDOMServerRenderer.render (../node_modules/react-dom/cjs/react-dom-server.node.development.js:3753:22)
  at ReactDOMServerRenderer.read (../node_modules/react-dom/cjs/react-dom-server.node.development.js:3690:29)
  at renderToString (../node_modules/react-dom/cjs/react-dom-server.node.development.js:4298:27)
  at Object.renderPage (../packages/next/dist/next-server/server/render.js:53:854)
  at Function.getInitialProps (integration/image-component/typescript/.next/server/pages/_document.js:625:19)
  at loadGetInitialProps (../packages/next/dist/next-server/lib/utils.js:5:101)
  at renderToHTML (../packages/next/dist/next-server/server/render.js:53:1145)
  "
  at Object.<anonymous> (integration/image-component/typescript/test/index.test.js:52:22)

packages/next/client/image.tsx

ijjk · 2021-06-17T19:33:28Z

Stats from current PR

Default Server Mode (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	jthcast/next.js canary	Change
buildDuration	11s	11s	-5ms
buildDurationCached	2.7s	2.6s	-4ms
nodeModulesSize	46.4 MB	46.4 MB	⚠️ +782 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	jthcast/next.js canary	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.009	2.01	0
/ avg req/sec	1244.32	1244.02	⚠️ -0.3
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.135	1.086	-0.05
/error-in-render avg req/sec	2202.13	2302.32	+100.19

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jthcast/next.js canary	Change
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	20.2 kB	20.2 kB	✓
webpack-HASH.js gzip	804 B	804 B	✓
Overall change	63 kB	63 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jthcast/next.js canary	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	jthcast/next.js canary	Change
_app-HASH.js gzip	801 B	801 B	✓
_error-HASH.js gzip	3.17 kB	3.17 kB	✓
amp-HASH.js gzip	527 B	527 B	✓
css-HASH.js gzip	329 B	329 B	✓
hooks-HASH.js gzip	903 B	903 B	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.65 kB	1.65 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	8.41 kB	8.41 kB	✓

Client Build Manifests

	vercel/next.js canary	jthcast/next.js canary	Change
_buildManifest.js gzip	391 B	391 B	✓
Overall change	391 B	391 B	✓

Rendered Page Sizes

	vercel/next.js canary	jthcast/next.js canary	Change
index.html gzip	524 B	524 B	✓
link.html gzip	536 B	536 B	✓
withRouter.html gzip	516 B	516 B	✓
Overall change	1.58 kB	1.58 kB	✓

Serverless Mode (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	jthcast/next.js canary	Change
buildDuration	12.6s	12.4s	-141ms
buildDurationCached	3.6s	3.6s	⚠️ +13ms
nodeModulesSize	46.4 MB	46.4 MB	⚠️ +782 B

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jthcast/next.js canary	Change
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	20.2 kB	20.2 kB	✓
webpack-HASH.js gzip	804 B	804 B	✓
Overall change	63 kB	63 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jthcast/next.js canary	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	jthcast/next.js canary	Change
_app-HASH.js gzip	801 B	801 B	✓
_error-HASH.js gzip	3.17 kB	3.17 kB	✓
amp-HASH.js gzip	527 B	527 B	✓
css-HASH.js gzip	329 B	329 B	✓
hooks-HASH.js gzip	903 B	903 B	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.65 kB	1.65 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	8.41 kB	8.41 kB	✓

Client Build Manifests

	vercel/next.js canary	jthcast/next.js canary	Change
_buildManifest.js gzip	391 B	391 B	✓
Overall change	391 B	391 B	✓

Serverless bundles Overall decrease ✓

	vercel/next.js canary	jthcast/next.js canary	Change
_error.js	16.9 kB	16.9 kB	✓
404.html	1.98 kB	1.98 kB	✓
500.html	1.96 kB	1.96 kB	✓
amp.amp.html	10.8 kB	10.8 kB	✓
amp.html	1.17 kB	1.17 kB	✓
css.html	1.35 kB	1.35 kB	✓
hooks.html	1.23 kB	1.23 kB	✓
index.js	17.2 kB	17.2 kB	✓
link.js	17.5 kB	17.5 kB	✓
routerDirect.js	17.3 kB	17.3 kB	✓
withRouter.js	17.3 kB	17.3 kB	-2 B
Overall change	105 kB	105 kB	-2 B

Webpack 4 Mode (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	jthcast/next.js canary	Change
buildDuration	9.7s	9.8s	⚠️ +167ms
buildDurationCached	3.9s	3.9s	-55ms
nodeModulesSize	46.4 MB	46.4 MB	⚠️ +782 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	jthcast/next.js canary	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.021	2.022	0
/ avg req/sec	1236.75	1236.18	⚠️ -0.57
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.126	1.157	⚠️ +0.03
/error-in-render avg req/sec	2220.72	2159.89	⚠️ -60.83

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jthcast/next.js canary	Change
677f882d2ed8..HASH.js gzip	13.3 kB	13.3 kB	✓
framework.HASH.js gzip	41.8 kB	41.8 kB	✓
main-HASH.js gzip	7.99 kB	7.99 kB	✓
webpack-HASH.js gzip	751 B	751 B	✓
Overall change	63.8 kB	63.8 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jthcast/next.js canary	Change
polyfills-HASH.js gzip	31.3 kB	31.3 kB	✓
Overall change	31.3 kB	31.3 kB	✓

Client Pages

	vercel/next.js canary	jthcast/next.js canary	Change
_app-HASH.js gzip	1.07 kB	1.07 kB	✓
_error-HASH.js gzip	3.84 kB	3.84 kB	✓
amp-HASH.js gzip	536 B	536 B	✓
css-HASH.js gzip	333 B	333 B	✓
hooks-HASH.js gzip	910 B	910 B	✓
index-HASH.js gzip	227 B	227 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	295 B	295 B	✓
withRouter-HASH.js gzip	292 B	292 B	✓
e025d2764813..52f.css gzip	125 B	125 B	✓
Overall change	9.28 kB	9.28 kB	✓

Client Build Manifests

	vercel/next.js canary	jthcast/next.js canary	Change
_buildManifest.js gzip	420 B	420 B	✓
Overall change	420 B	420 B	✓

Rendered Page Sizes

	vercel/next.js canary	jthcast/next.js canary	Change
index.html gzip	567 B	567 B	✓
link.html gzip	579 B	579 B	✓
withRouter.html gzip	561 B	561 B	✓
Overall change	1.71 kB	1.71 kB	✓

Commit: 10f7060

…26166) ## Bug - [x] Related issues linked using `fixes vercel#26135 ` - [x] Integration tests added fixes vercel#26135 ## Feature - [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [ ] Related issues linked using `fixes vercel#26135 ` - [ ] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. ## Documentation / Examples - [ ] Make sure the linting passes

Fix Show error when user put wrong values in width or height

56fa669

jthcast requested review from divmain, ijjk, lfades, shuding and timneutkens as code owners June 16, 2021 06:17

ijjk added the type: next label Jun 16, 2021

timneutkens requested changes Jun 16, 2021

View reviewed changes

atcastle previously approved these changes Jun 16, 2021

View reviewed changes

Add test

3446c9b

jthcast dismissed atcastle’s stale review via 3446c9b June 16, 2021 21:20

jthcast requested review from timneutkens and atcastle June 16, 2021 21:25

Merge branch 'canary' into canary

45f75c5

timneutkens reviewed Jun 17, 2021

View reviewed changes

jthcast added 3 commits June 17, 2021 21:56

Fix handle another case

b9e2b7d

Merge branch 'canary' of https://github.com/jthcast/next.js into canary

ae6c75b

Merge branch 'canary' into canary

9b4e17b

jthcast requested a review from timneutkens June 17, 2021 13:03

styfle reviewed Jun 17, 2021

View reviewed changes

test/integration/image-component/default/pages/invalid-weight-or-height.js Outdated Show resolved Hide resolved

styfle reviewed Jun 17, 2021

View reviewed changes

test/integration/image-component/default/pages/invalid-weight-or-height.js Outdated Show resolved Hide resolved

styfle reviewed Jun 17, 2021

View reviewed changes

packages/next/client/image.tsx Outdated Show resolved Hide resolved

jthcast and others added 2 commits June 18, 2021 00:14

Update test/integration/image-component/default/pages/invalid-weight-…

e9c55d6

…or-height.js Co-authored-by: Steven <steven@ceriously.com>

Update packages/next/client/image.tsx

5189265

Co-authored-by: Steven <steven@ceriously.com>

styfle reviewed Jun 17, 2021

View reviewed changes

test/integration/image-component/default/test/index.test.js Outdated Show resolved Hide resolved

jthcast and others added 3 commits June 18, 2021 00:18

Update test/integration/image-component/default/test/index.test.js

3d85b69

Co-authored-by: Steven <steven@ceriously.com>

Fix rename weight to width

d344a0d

Merge branch 'canary' into canary

eba3c73

jthcast requested a review from styfle June 17, 2021 15:24

This comment has been minimized.

Sign in to view

jthcast added 3 commits June 18, 2021 02:02

Merge branch 'canary' into canary

0ea1336

Fix intrinsic need width and height value

51f4ba8

Merge branch 'canary' of https://github.com/jthcast/next.js into canary

9c84023

jthcast dismissed ijjk’s stale review via 9c84023 June 17, 2021 17:05

This comment has been minimized.

Sign in to view

jthcast requested review from ijjk, timneutkens and styfle June 17, 2021 17:51

Merge branch 'canary' into canary

6ce4b62

styfle reviewed Jun 17, 2021

View reviewed changes

test/integration/image-component/default/pages/invalid-placeholder-blur-static.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jthcast added 2 commits June 18, 2021 03:20

Fix static image doesn't need width and height

559e8da

Merge branch 'canary' of https://github.com/jthcast/next.js into canary

3cad23c

styfle previously approved these changes Jun 17, 2021

View reviewed changes

Merge branch 'canary' into canary

168597d

This comment has been minimized.

Sign in to view

jthcast added 2 commits June 18, 2021 03:56

Fix lint

0427b1d

Merge branch 'canary' of https://github.com/jthcast/next.js into canary

eda2c84

jthcast dismissed styfle’s stale review via eda2c84 June 17, 2021 18:57

styfle reviewed Jun 17, 2021

View reviewed changes

packages/next/client/image.tsx Outdated Show resolved Hide resolved

Fix comparison

10f7060

styfle approved these changes Jun 17, 2021

View reviewed changes

ijjk approved these changes Jun 17, 2021

View reviewed changes

kodiakhq bot merged commit 51022d5 into vercel:canary Jun 17, 2021

sync-by-unito bot mentioned this pull request Jun 23, 2021

chore(deps): bump next from 10.1.2 to 11.0.1 filecoin-project/slate#799

Closed

vercel locked as resolved and limited conversation to collaborators Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Show error when user put wrong values in width or height #26166

Fix Show error when user put wrong values in width or height #26166

jthcast commented Jun 16, 2021 •

edited

timneutkens left a comment

atcastle left a comment

jthcast commented Jun 16, 2021

timneutkens Jun 17, 2021

jthcast Jun 17, 2021 •

edited

atcastle Jun 17, 2021

jthcast Jun 17, 2021 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Jun 17, 2021

ijjk commented Jun 17, 2021

	if (Number.isNaN(widthInt) \|\| Number.isNaN(heightInt)){
	if (layout !== 'fill' && (Number.isNaN(widthInt) \|\| Number.isNaN(heightInt)){

Fix Show error when user put wrong values in width or height #26166

Fix Show error when user put wrong values in width or height #26166

Conversation

jthcast commented Jun 16, 2021 • edited

Bug

Feature

Documentation / Examples

timneutkens left a comment

Choose a reason for hiding this comment

atcastle left a comment

Choose a reason for hiding this comment

jthcast commented Jun 16, 2021

timneutkens Jun 17, 2021

Choose a reason for hiding this comment

jthcast Jun 17, 2021 • edited

Choose a reason for hiding this comment

atcastle Jun 17, 2021

Choose a reason for hiding this comment

jthcast Jun 17, 2021 • edited

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Jun 17, 2021

Failing test suites

ijjk commented Jun 17, 2021

Stats from current PR

jthcast commented Jun 16, 2021 •

edited

jthcast Jun 17, 2021 •

edited

jthcast Jun 17, 2021 •

edited