Skip to content

[PoC] fix: improve Response.json() and Response.redirect() spec compliance and efficiency#343

Open
usualoma wants to merge 1 commit intov2from
perf-v2-json
Open

[PoC] fix: improve Response.json() and Response.redirect() spec compliance and efficiency#343
usualoma wants to merge 1 commit intov2from
perf-v2-json

Conversation

@usualoma
Copy link
Copy Markdown
Member

The optimizations for Response.json() and Response.redirect() introduced in #333 cause behavior discrepancies with native Response objects in cases such as those discussed at honojs/hono#2343.

While excluding the Response.json() optimization is under consideration, if it is included as is, I believe we should ensure that there are no behavioral discrepancies with the native Response object, as proposed in this pull request.

What this pull request includes

  • Throw TypeError for non-JSON-serializable data in Response.json()
  • Add URL validation with fast-path for common ASCII URLs in Response.redirect()
  • Remove redundant type assertions in Response.json()

…and efficiency

- Throw TypeError for non-JSON-serializable data in Response.json()
- Add URL validation with fast-path for common ASCII URLs in Response.redirect()
- Remove redundant type assertions in Response.json()
@yusukebe
Copy link
Copy Markdown
Member

Hey @usualoma

I have reconsidered. First, I confirmed that the performance of Response.json will be significantly improved with the optimization. Here is one of the benchmark results:

CleanShot 2026-04-14 at 18 10 32@2x

I said these changes are unnecessary, but the improvement is significant, and the code hasn't increased much. So, let's keep the optimizations for Response.json() and Response.redirect()!

Is this PR ready for review?

@usualoma
Copy link
Copy Markdown
Member Author

Hi @yusukebe

Thanks!

I hadn't really thought this through because I wasn't sure if the idea would be adopted, but writing it out with a straightforward if statement might actually be faster than using Uint8Array(128). Let me check—please hold on a moment.

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