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

Improve types for <Image /> and responseLimit #40863

Merged
merged 12 commits into from Sep 27, 2022
Merged

Improve types for <Image /> and responseLimit #40863

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9002c1d
feat: :label: stronger types for `width`, `height`, `quality` on `<Im…
brunocrosier Sep 23, 2022
41bf5b3
feat: :label: stronger types for `responseLimit` and `sizeLimit`
brunocrosier Sep 23, 2022
2b48831
add `tb` `TB` `pb` `PB`
brunocrosier Sep 23, 2022
f73ba5a
Merge branch 'canary' into canary
brunocrosier Sep 23, 2022
8400ca9
add invalid examples for tests
brunocrosier Sep 26, 2022
f496180
add `alt` prop to valid fixtures
brunocrosier Sep 26, 2022
563c4db
add valid fixtures for `numeric-string-types`
brunocrosier Sep 26, 2022
e35879d
rename `NumericValue` to `SafeNumber`
brunocrosier Sep 26, 2022
1565b04
Merge branch 'canary' into canary
brunocrosier Sep 26, 2022
f79a201
Merge branch 'canary' into canary
styfle Sep 27, 2022
e0f7813
Merge branch 'canary' into canary
kodiakhq[bot] Sep 27, 2022
1d26e99
Merge branch 'canary' into canary
ijjk Sep 27, 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
8 changes: 5 additions & 3 deletions packages/next/client/future/image.tsx
View file
Open in desktop
Expand Up @@ -72,6 +72,8 @@ interface StaticRequire {

type StaticImport = StaticRequire | StaticImageData

type SafeNumber = number | `${number}`

function isStaticRequire(
src: StaticRequire | StaticImageData
): src is StaticRequire {
Expand All @@ -98,11 +100,11 @@ export type ImageProps = Omit<
> & {
src: string | StaticImport
alt: string
width?: number | string
height?: number | string
width?: SafeNumber
height?: SafeNumber
fill?: boolean
loader?: ImageLoader
quality?: number | string
quality?: SafeNumber
priority?: boolean
loading?: LoadingValue
placeholder?: PlaceholderValue
Expand Down
8 changes: 5 additions & 3 deletions packages/next/client/image.tsx
View file
Open in desktop
Expand Up @@ -218,6 +218,8 @@ interface StaticRequire {

type StaticImport = StaticRequire | StaticImageData

type SafeNumber = number | `${number}`

function isStaticRequire(
src: StaticRequire | StaticImageData
): src is StaticRequire {
Expand All @@ -243,11 +245,11 @@ export type ImageProps = Omit<
'src' | 'srcSet' | 'ref' | 'width' | 'height' | 'loading'
> & {
src: string | StaticImport
width?: number | string
height?: number | string
width?: SafeNumber
height?: SafeNumber
layout?: LayoutValue
loader?: ImageLoader
quality?: number | string
quality?: SafeNumber
priority?: boolean
loading?: LoadingValue
lazyRoot?: React.RefObject<HTMLElement> | null
Expand Down
6 changes: 3 additions & 3 deletions packages/next/server/api-utils/node.ts
View file
Open in desktop
@@ -1,6 +1,6 @@
import type { IncomingMessage, ServerResponse } from 'http'
import type { NextApiRequest, NextApiResponse } from '../../shared/lib/utils'
import type { PageConfig } from 'next/types'
import type { PageConfig, ResponseLimit, SizeLimit } from 'next/types'
import {
checkIsManualRevalidate,
PRERENDER_REVALIDATE_ONLY_GENERATED_HEADER,
Expand Down Expand Up @@ -140,7 +140,7 @@ function parseJson(str: string): object {
*/
export async function parseBody(
req: IncomingMessage,
limit: string | number
limit: SizeLimit
): Promise<any> {
let contentType
try {
Expand Down Expand Up @@ -182,7 +182,7 @@ type ApiContext = __ApiPreviewProps & {
revalidate?: (_req: IncomingMessage, _res: ServerResponse) => Promise<any>
}

function getMaxContentLength(responseLimit?: number | string | boolean) {
function getMaxContentLength(responseLimit?: ResponseLimit) {
if (responseLimit && typeof responseLimit !== 'boolean') {
return bytes.parse(responseLimit)
}
Expand Down
3 changes: 2 additions & 1 deletion packages/next/server/base-http/node.ts
View file
Open in desktop
@@ -1,5 +1,6 @@
import type { ServerResponse, IncomingMessage } from 'http'
import type { Writable, Readable } from 'stream'
import type { SizeLimit } from 'next/types'

import { NextApiRequestCookies, SYMBOL_CLEARED_COOKIES } from '../api-utils'
import { parseBody } from '../api-utils/node'
Expand Down Expand Up @@ -34,7 +35,7 @@ export class NodeNextRequest extends BaseNextRequest<Readable> {
super(_req.method!.toUpperCase(), _req.url!, _req)
}

async parseBody(limit: string | number): Promise<any> {
async parseBody(limit: SizeLimit): Promise<any> {
return parseBody(this._req, limit)
}
}
Expand Down
24 changes: 22 additions & 2 deletions packages/next/types/index.d.ts
View file
Open in desktop
Expand Up @@ -61,6 +61,22 @@ export type Redirect =
*/
export type NextPage<P = {}, IP = P> = NextComponentType<NextPageContext, IP, P>

export type FileSizeSuffix = `${
| 'k'
| 'K'
| 'm'
| 'M'
| 'g'
| 'G'
| 't'
| 'T'
| 'p'
| 'P'}${'b' | 'B'}`
Copy link
Member

Choose a reason for hiding this comment

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

This seems to restrictive because it doesnt work with 1000 which is documented in the JSDoc:

any string format supported by bytes, for example 1000, '500kb'

Lets add tests for this to ensure all cases are covered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! 🙏🏼

Hmm, I am pretty sure that number still works here (see screenshot)

image

Basically, SizeLimit is either a number or a literal type comprised of a number and an accepted file suffix

Essentially, this should match exactly to the types from the bytes library (screenshot below)

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are some examples of it working as expected:

image

I can try to add some tests for this, do you know where I can add these?


export type SizeLimit = number | `${number}${FileSizeSuffix}`

export type ResponseLimit = SizeLimit | boolean

/**
* `Config` type, use it for export const config
*/
Expand All @@ -72,12 +88,16 @@ export type PageConfig = {
* any string format supported by `bytes`, for example `1000`, `'500kb'` or
* `'3mb'`.
*/
responseLimit?: number | string | boolean
responseLimit?: ResponseLimit
/**
* The byte limit of the body. This is the number of bytes or any string
* format supported by `bytes`, for example `1000`, `'500kb'` or `'3mb'`.
*/
bodyParser?: { sizeLimit?: number | string } | false
bodyParser?:
| {
sizeLimit?: SizeLimit
}
| false
/**
* Flag to disable warning "API page resolved
* without sending a response", due to explicitly
Expand Down
18 changes: 18 additions & 0 deletions test/integration/image-component/typescript/pages/invalid.tsx
View file
Open in desktop
Expand Up @@ -26,6 +26,24 @@ const Invalid = () => {
height="500"
placeholder="invalid"
></Image>
<Image
id="invalid-width-string-type"
alt="invalid-width-string-type"
src="https://image-optimization-test.vercel.app/test.jpg"
width="500foo"
/>
<Image
id="invalid-height-string-type"
alt="invalid-height-string-type"
src="https://image-optimization-test.vercel.app/test.jpg"
height="500bar"
/>
<Image
id="invalid-quality-string-type"
alt="invalid-quality-string-type"
src="https://image-optimization-test.vercel.app/test.jpg"
quality="500baz"
/>
<p id="stubtext">This is the invalid usage</p>
</div>
)
Expand Down
8 changes: 8 additions & 0 deletions test/integration/image-component/typescript/pages/valid.tsx
View file
Open in desktop
Expand Up @@ -43,6 +43,14 @@ const Page = () => {
width={500}
height={500}
/>
<Image
id="numeric-string-types"
alt="numeric-string-types"
src="https://image-optimization-test.vercel.app/test.jpg"
quality="80"
width="500"
height="500"
/>
<Image
id="data-protocol"
src="data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs="
Expand Down
18 changes: 18 additions & 0 deletions test/integration/image-future/typescript/pages/invalid.tsx
View file
Open in desktop
Expand Up @@ -27,6 +27,24 @@ const Invalid = () => {
height="500"
placeholder="invalid"
/>
<Image
id="invalid-width-string-type"
alt="invalid-width-string-type"
src="https://image-optimization-test.vercel.app/test.jpg"
width="500foo"
/>
<Image
id="invalid-height-string-type"
alt="invalid-height-string-type"
src="https://image-optimization-test.vercel.app/test.jpg"
height="500bar"
/>
<Image
id="invalid-quality-string-type"
alt="invalid-quality-string-type"
src="https://image-optimization-test.vercel.app/test.jpg"
quality="500baz"
/>
<Image
id="missing-alt"
src="https://image-optimization-test.vercel.app/test.jpg"
Expand Down
10 changes: 10 additions & 0 deletions test/integration/image-future/typescript/pages/valid.tsx
View file
Open in desktop
Expand Up @@ -28,13 +28,15 @@ const Page = () => {
id="fill-no-width-and-height"
src="https://image-optimization-test.vercel.app/test.jpg"
fill
alt=""
/>
<Image
id="quality-num"
src="https://image-optimization-test.vercel.app/test.jpg"
quality={80}
width={500}
height={500}
alt=""
/>
<Image
id="quality-str"
Expand All @@ -44,6 +46,14 @@ const Page = () => {
width={500}
height={500}
/>
<Image
id="numeric-string-types"
alt="numeric-string-types"
src="https://image-optimization-test.vercel.app/test.jpg"
quality="80"
width="500"
height="500"
/>
<Image
id="data-protocol"
alt="data-protocol"
Expand Down