fix: More strictly, determine when new URL() should be used by usualoma · Pull Request #310 · honojs/node-server

usualoma · 2026-03-01T06:33:52Z

This PR tightens the fast-path eligibility introduced in #301 for request URL handling.

The optimization remains performance-focused, but the conditions for taking the fast path are now more strict to avoid unsafe edge cases.
In short: this is slightly slower than #301 in some cases, but it is the best balance we can achieve while keeping behavior correct and performance impact minimal.

benchmarks

improve-normalize-url (This PR's branch)

============================================================
BENCHMARK RESULTS
============================================================

| Benchmark         | npm            | dev            | Difference  |
| ----------------- | -------------- | -------------- | ----------- |
| Average           | 48,671.60      | 57,971.12      | +19.11%     |
| Ping (GET /)      | 63,550.47      | 66,520.31      | +4.67%      |
| Query (GET /id)   | 61,283.56      | 63,584.95      | +3.76%      |
| Body (POST /json) | 21,180.76      | 43,808.09      | +106.83%    |

pref/request-v2 (base branch)

============================================================
BENCHMARK RESULTS
============================================================

| Benchmark         | npm            | dev            | Difference  |
| ----------------- | -------------- | -------------- | ----------- |
| Average           | 48,239.95      | 59,463.41      | +23.27%     |
| Ping (GET /)      | 62,649.83      | 67,914.39      | +8.40%      |
| Query (GET /id)   | 60,818.91      | 65,278.81      | +7.33%      |
| Body (POST /json) | 21,251.12      | 45,197.04      | +112.68%    |

usualoma · 2026-03-01T06:40:51Z

Hi @yusukebe, would you please review this?

Hi @mgcrea, if you have any comments, I’d really appreciate your feedback.

yusukebe · 2026-03-05T11:09:20Z

src/request.ts

How about creating url.ts and url.test.ts to include the code for a URL?

You're absolutely right! I've moved it to a separate file.
9d03243

yusukebe · 2026-03-06T07:29:13Z

src/url.ts

+      urlObj.hostname.length !== host.length &&
+      urlObj.hostname !== (host.includes(':') ? host.replace(/:\d+$/, '') : host).toLowerCase()
+    ) {
+      throw 'Invalid host header'


Throwing a string is intended? (I think it's intended because it's you!) Are there any concerns about this? If no, I think it's okay!

Thanks for pointing that out.
Yeah, it might not be the best way to write general code. (Because it confuses readers like you.)

In the actual flow of the code, I wanted to change this:

node-server/src/request.ts

Lines 323 to 324 in 606a905

if (typeof e === 'string') {

throw new RequestError(e)

I wanted to use a plain RequestError like this. (Because this isn't "an invalid URL", but rather "an invalid hostname" being specified by Host header.)

RequestError(‘Invalid host header’);

Well, code that throws a string definitely has a bad smell.

What about a refactoring such as 10b44e0?

yusukebe · 2026-03-06T08:22:17Z

test/url.test.ts

@@ -0,0 +1,70 @@
+import { RequestError } from '../src/error'


../src/error is right? Or did you forget to push error.ts?

? The CI does not work now!

Sorry for the trouble!
Added a file at bc49af9.

Oh, right—under the current setup, CI doesn't run on branches outside of main.

yusukebe · 2026-03-06T08:49:45Z

test/request.test.ts

+          },
+          url: '/foo.txt',
+        } as IncomingMessage)
+      }).toThrow(RequestError)


It's better to compare also the error content Invalid host header. I think you can use the RequestError('Invalid host header') instance.

Thank you!
updated: dd5a383

yusukebe

LGTM!

yusukebe · 2026-03-07T05:04:58Z

@usualoma

Nice! I'll merge this into https://github.com/honojs/node-server/tree/pref/request-v2. Thanks!

usualoma added 3 commits March 1, 2026 15:26

fix: More strictly, determine when new URL() should be used

b46cd0e

fix: compare host case-insensitively

de8c772

test: Add test cases for URL normalization

9147e43

yusukebe reviewed Mar 5, 2026

View reviewed changes

usualoma added 2 commits March 5, 2026 22:40

refactor: separate URL normalization process code into separate files

9d03243

refactor(url): improve error class compatibility

606a905

yusukebe reviewed Mar 6, 2026

View reviewed changes

refactor: improve exception throwing

10b44e0

yusukebe reviewed Mar 6, 2026

View reviewed changes

fix: add missing src/error.ts

bc49af9

yusukebe reviewed Mar 6, 2026

View reviewed changes

test: improve test for error throwing

dd5a383

yusukebe approved these changes Mar 7, 2026

View reviewed changes

yusukebe merged commit 68a5815 into pref/request-v2 Mar 7, 2026

yusukebe deleted the improve-normalize-url branch March 7, 2026 05:05

Conversation

usualoma commented Mar 1, 2026

benchmarks

Uh oh!

usualoma commented Mar 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yusukebe left a comment

Choose a reason for hiding this comment

Uh oh!

yusukebe commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants