Skip to content

feat(context): infer TypedResponse for TextRespond#2579

Closed
NamesMT wants to merge 5 commits intohonojs:nextfrom
NamesMT:patch-1
Closed

feat(context): infer TypedResponse for TextRespond#2579
NamesMT wants to merge 5 commits intohonojs:nextfrom
NamesMT:patch-1

Conversation

@NamesMT
Copy link
Copy Markdown
Contributor

@NamesMT NamesMT commented Apr 30, 2024

Author should do the followings, if applicable

  • Add tests Adjusted tests
  • Run tests - Ran all tests except for deno and wrangler because I don't have them
  • yarn denoify to generate files for Deno

@yusukebe yusukebe added the v4.3 label Apr 30, 2024
@yusukebe
Copy link
Copy Markdown
Member

Hi @NamesMT

This is awesome. I'd like to merge it into the "next" branch for the next minor release will be shipped soon.

I've fixed that res.text() and res.json() on the client side did not return the correct types with 9d690a6. Now, these are correct:

CleanShot 2024-05-01 at 08 04 16@2x

CleanShot 2024-05-01 at 08 04 28@2x

I think this is good. How about you?

@yusukebe yusukebe changed the base branch from main to next April 30, 2024 23:09
@NamesMT
Copy link
Copy Markdown
Contributor Author

NamesMT commented May 1, 2024

Thanks @yusukebe, that's great.

But please wait a bit before merging, I found an untested regression, will document and add tests for it maybe tonight, urgent have to go now.

@yusukebe
Copy link
Copy Markdown
Member

yusukebe commented May 1, 2024

@NamesMT Okay!

NamesMT added a commit to NamesMT/hono that referenced this pull request May 1, 2024
@NamesMT
Copy link
Copy Markdown
Contributor Author

NamesMT commented May 1, 2024

Hi @yusukebe, I've done the updates to fix the regression and improve the type inferring for the Response.

The regression is: c.json() is able to return string JSONPrimitive, in the latest update to res.json() they were broken and always typed as never.

The final scope of work to get everything perfectly working was much bigger than I thought and I don't know if I did it the best way, so I will create a new draft PR for it to discuss if it's good or not.

It's an un-splitted commit but heres the readable updates that I've made:
// Edit for information: Simplify is copied over from type-fest

  • Switch from InterfaceToType to Simplify for better performance + easier working with types, InterfaceToType was yelling Type instantiation is excessively deep and possibly infinite.
  • TypedResponse: add ResponseFormat generic ('json' | 'text')
  • ClientResponse: accept ResponseFormat generic to properly return the correct type
  • Endpoint (and related interfaces): now includes outputFormat: ResponseFormat

Bonus:

  • ClientResponse now properly extends globalThis.Response for better experience when working with functions that expects a fetch's Response

+ add new + update related tests

New PR: #2581

@yusukebe
Copy link
Copy Markdown
Member

yusukebe commented May 2, 2024

Hi @NamesMT

That's super great. Anyway, I want to go with #2581 though we may have improvements. Let's talk on #2581

@NamesMT
Copy link
Copy Markdown
Contributor Author

NamesMT commented May 2, 2024

Close to continue in #2581

@NamesMT NamesMT closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants