Conversation
Add off(method, route) method to remove registered route handlers.
Uses rou3's removeRoute under the hood and also cleans up the internal
~routes array.
app.get("/temp", () => "hello");
app.off("GET", "/temp"); // route removed, returns 404
Closes #1159
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/router.test.ts (1)
225-252: LGTM!The tests cover the core functionality well: basic route removal and method-specific removal. The test structure follows the established patterns with
describeMatrix.Consider adding edge case coverage for completeness:
- Removing a non-existent route (no-op behavior)
- Using lowercase method strings (e.g.,
off("get", "/path"))- Verifying chaining works (e.g.,
t.app.off(...).off(...))💡 Example additional test
it("handles lowercase method and non-existent routes gracefully", async () => { t.app.get("/lower", () => "ok"); // Lowercase method should work t.app.off("get", "/lower"); const res = await t.fetch("/lower"); expect(res.status).toBe(404); // Removing non-existent route should not throw expect(() => t.app.off("GET", "/does-not-exist")).not.toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/router.test.ts` around lines 225 - 252, Add tests covering edge cases for the router off behavior: verify that calling t.app.off with a lowercase method string (e.g., "get") removes the route the same as uppercase, that removing a non-existent route is a no-op and does not throw, and that off returns the app (or is chainable) so chaining t.app.off(...).off(...) works; update the test file's "off (remove route)" suite to add one spec that registers a route with t.app.get, calls t.app.off("get", "/path") and asserts 404 on fetch, asserts that calling t.app.off("GET", "/does-not-exist") does not throw, and asserts chaining by checking the return value of t.app.off is the app (or by chaining two off calls and verifying expected outcome).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/h3.ts`:
- Around line 153-164: The off method currently calls removeRoute(this["~rou3"],
_method, route) which deletes all matching entries from rou3 but then uses
findIndex + splice on this["~routes"], removing only the first match and leaving
stale entries; update off (the function handling route removal) to filter
this["~routes"] and remove every entry where r.route === route and (no _method
or r.method === _method) so the internal ~routes array stays in sync with
removeRoute/rou3.
---
Nitpick comments:
In `@test/router.test.ts`:
- Around line 225-252: Add tests covering edge cases for the router off
behavior: verify that calling t.app.off with a lowercase method string (e.g.,
"get") removes the route the same as uppercase, that removing a non-existent
route is a no-op and does not throw, and that off returns the app (or is
chainable) so chaining t.app.off(...).off(...) works; update the test file's
"off (remove route)" suite to add one spec that registers a route with
t.app.get, calls t.app.off("get", "/path") and asserts 404 on fetch, asserts
that calling t.app.off("GET", "/does-not-exist") does not throw, and asserts
chaining by checking the return value of t.app.off is the app (or by chaining
two off calls and verifying expected outcome).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 563496ba-9d27-4506-82ad-637179584dae
📒 Files selected for processing (3)
src/h3.tssrc/types/h3.tstest/router.test.ts
src/h3.ts
Outdated
| off(method: HTTPMethod | Lowercase<HTTPMethod> | "", route: string): this { | ||
| const _method = (method || "").toUpperCase(); | ||
| route = new URL(route, "http://_").pathname; | ||
| removeRoute(this["~rou3"], _method, route); | ||
| const idx = this["~routes"].findIndex( | ||
| (r) => r.route === route && (!_method || r.method === _method), | ||
| ); | ||
| if (idx !== -1) { | ||
| this["~routes"].splice(idx, 1); | ||
| } | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
rou3 removeRoute behavior with duplicate routes
💡 Result:
In h3js/rou3 (JS), duplicate routes (same method + same path pattern) are allowed:
addRoute()stores routes in an array per node+method and pushes each new entry, so duplicates accumulate rather than overwrite. [1]findRoute()returns only the first match for that method/path (it returnsmatch[0]), so with duplicates you’ll get the earliest-added handler/data. [2]findAllRoutes()returns all matches, including duplicates (it spreads/pushes the whole per-method array into results). [3]removeRoute()deletesnode.methods[method]entirely (not a single array element), so it removes all duplicates registered at that exact node+method in one call. There’s no API to remove “just one” of the duplicates. [4]
Sources:
[1] add.ts route storage (node.methods![method] ??= []; ... push(...)) (github.com)
[2] find.ts returns first match (return staticMatch[0], ...?.[0]) (raw.githubusercontent.com)
[3] find-all.ts pushes all matches (matches.push(...match)) (raw.githubusercontent.com)
[4] remove.ts deletes method entry (delete node.methods[method]) (raw.githubusercontent.com)
Citations:
- 1: https://github.com/h3js/rou3/blob/main/src/operations/add.ts
- 2: https://raw.githubusercontent.com/h3js/rou3/main/src/operations/find.ts
- 3: https://raw.githubusercontent.com/h3js/rou3/main/src/operations/find-all.ts
- 4: https://raw.githubusercontent.com/h3js/rou3/main/src/operations/remove.ts
Remove all matching route entries, not just the first.
The removeRoute() call removes all matching entries from rou3, but findIndex + splice removes only the first matching entry from ~routes. When a route is registered multiple times with the same method and path, this leaves stale entries in the internal ~routes array that become out of sync with rou3.
Use filter to remove all matching entries consistently:
Proposed fix
off(method: HTTPMethod | Lowercase<HTTPMethod> | "", route: string): this {
const _method = (method || "").toUpperCase();
route = new URL(route, "http://_").pathname;
removeRoute(this["~rou3"], _method, route);
- const idx = this["~routes"].findIndex(
- (r) => r.route === route && (!_method || r.method === _method),
- );
- if (idx !== -1) {
- this["~routes"].splice(idx, 1);
- }
+ this["~routes"] = this["~routes"].filter(
+ (r) => !(r.route === route && (!_method || r.method === _method)),
+ );
return this;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/h3.ts` around lines 153 - 164, The off method currently calls
removeRoute(this["~rou3"], _method, route) which deletes all matching entries
from rou3 but then uses findIndex + splice on this["~routes"], removing only the
first match and leaving stale entries; update off (the function handling route
removal) to filter this["~routes"] and remove every entry where r.route ===
route and (no _method or r.method === _method) so the internal ~routes array
stays in sync with removeRoute/rou3.
|
We ca introduce a |
Per maintainer review: avoid adding methods to H3 class to keep
bundle size small. removeRoute is now a tree-shakeable standalone
utility function instead of app.off().
Usage:
import { removeRoute } from "h3";
removeRoute(app, "GET", "/temp");
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/route.remove.ts`:
- Around line 25-27: The findIndex predicate treats empty-string _method as
falsy, so (!_method || r.method === _method) matches all routes and can remove
e.g. GET/POST entries from app["~routes"]; update the predicate to only treat
null/undefined as the wildcard by replacing (!_method || r.method === _method)
with (_method == null || r.method === _method) in the findIndex call
(referencing idx, app["~routes"], r.route, r.method and _method) so only truly
absent methods match the methodless/all-method entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a59f34fa-16c2-49da-819f-705948fdfe21
📒 Files selected for processing (4)
src/index.tssrc/utils/route.remove.tstest/router.test.tstest/unit/package.test.ts
pi0
changed the title
removeRoute
2 participants
Summary
Closes #1159
Adds
removeRouteutility to remove registered route handlers.Implementation
removeRoute()to remove from the router context~routesarray for consistencyremoveRoute(app, "GET", "/path")only removes GET, leaves POST etc.Test plan
describeMatrixSummary by CodeRabbit
Release Notes