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(context): infer TypedResponse for TextRespond #2579

Closed
wants to merge 5 commits into from

Conversation

NamesMT
Copy link
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
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
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
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
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
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
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.

None yet

2 participants