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

feat(utils): specify detailed return type for parseBody #2771

Merged
merged 13 commits into from
May 24, 2024

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented May 23, 2024

What is this?

More restrictive on the type of value returned from parseBody().

const value = (await parseBody(req))['key'] // => string | File
const maybeArray = (await parseBody(req, {all: true}))['key'] // => string | File | (string | File)[]
const maybeObject = (await parseBody(req, {dot: true}))['key'] // => string | File | Object

Specification Changes

In v4.3.x, BodyData was defined as follows

export type BodyData = Record<string, string | File | (string | File)[]>

For applications using it directly, this change results in the following ((string | File)[] is no longer included)

const data: BodyData = {} // means Record<string, string | File>

To get the same result as in v4.3.x, you need to do the following.

const data: BodyData<{all: true}> = {}

Although there are some changes from v4.3.x as described above, I think it is useful to restrict the type of parseBody(), so I think it is better to include this change.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun denoify to generate files for Deno
  • bun run format:fix && bun run lint:fix to format the code

@usualoma
Copy link
Member Author

Uh, it wasn't easy. I'll think about it a bit.

@@ -1,4 +1,9 @@
import { parseBody } from './body'
import type { BodyData } from './body'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool PR!

I think you can just merge the imports in one import like this

import { parseBody, type BodyData } from './body'

I think we need to setup eslint to avoid imports to the same file @usualoma @yusukebe

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! That's certainly better.
I made the change in eecfc43.

Copy link
Member

Choose a reason for hiding this comment

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

@fzn0x

I think we need to setup eslint to avoid imports to the same file @usualoma @yusukebe

That's great! I'll work on it soon.

@usualoma
Copy link
Member Author

@yusukebe How about this?

@usualoma usualoma changed the title Feat type for parse body feat(utils): specify detailed return type for parseBody May 23, 2024
@usualoma
Copy link
Member Author

Also added type to c.req.parseBody() in 3fb0ef8

@yusukebe
Copy link
Member

Hi @usualoma !

It would be nice to have the type definition determined by what the value of the option is. But I think there are two problems:

1. Annotations by IDE are not friendly.

As you can see, it's not easy to understand type definitions for the user, though it should annotate string | File | (string | File)[]

CleanShot 2024-05-23 at 15 15 07@2x

CleanShot 2024-05-23 at 15 15 33@2x

2. I intended the first argument for the generic be parsed object types

I designed it so that the first argument of parseBody() generics is the returned type:

const result = await parseBody<{ key1: string, key2: string }>()
// ^ result should be { key1: string, key2: string }

But with this PR, it will be typed for the options. This will break the spec.

https://github.com/honojs/hono/pull/2771/files#diff-fb57c6d8981c47b991f2dc7914e05041c7d0c6651eabc7145562c00a78bc93d0R63-R72


I'm investigating how to fix these problems, but I can't find a good way now 😦

FYI

You may already know that Simply is a very useful utility type even though it can't resolve the problem 1.

https://github.com/honojs/hono/blob/main/src/utils/types.ts#L40

https://github.com/sindresorhus/type-fest/blob/main/source/simplify.d.ts

@usualoma
Copy link
Member Author

@yusukebe Thank you!

  1. I intended the first argument for the generic be parsed object types

Sorry, the very first push, 288e050, had that problem, but I think it was fixed in the subsequent 3b178fc.

I also added an explicit test at a6c300d. That should work, right?

  1. Annotations by IDE are not friendly.

This is indeed true. I don't know how to solve it and will investigate.

@yusukebe
Copy link
Member

@usualoma

I also added an explicit test at a6c300d. That should work, right?

Yes! Thanks!

This is indeed true. I don't know how to solve it and will investigate.

The one way is to write the types as very redundancy, but it might not be the best solution. Hmm.

@usualoma usualoma force-pushed the feat-type-for-parseBody branch from a2b9d42 to e6aa84e Compare May 23, 2024 09:29
@usualoma
Copy link
Member Author

Hi @yusukebe
How about 0deb075?
It will appear as follows

I wanted to change the name of the x shown as [x: string], but I couldn't figure out how to contrl the variable name if I wrote [K in keyof T], so I left it as it is, named x.

CleanShot 2024-05-23 at 18 23 01@2x

CleanShot 2024-05-23 at 18 23 14@2x

CleanShot 2024-05-23 at 18 23 27@2x

CleanShot 2024-05-23 at 18 23 40@2x

@yusukebe
Copy link
Member

@usualoma

Awesome!! Let's go with this!

@yusukebe yusukebe added the v4.4 label May 23, 2024
@usualoma
Copy link
Member Author

In 1c18227, variable names were changed to appropriate names. My work is now complete!

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

@usualoma Great! Merging now.

@yusukebe yusukebe merged commit 406abbb into honojs:next May 24, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants