fix(hono): Distinguish .use() middleware in sub-apps from .all() handlers#20554
Merged
fix(hono): Distinguish .use() middleware in sub-apps from .all() handlers#20554
.use() middleware in sub-apps from .all() handlers#20554Conversation
s1gr1d
changed the title
.use() middleware from .all() handlers.use() middleware in sub-apps from .all() handlers
Contributor
size-limit report 📦
|
chargome
approved these changes
Apr 28, 2026
Member
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Few minor things, but generally LGTM!
| * (those are the middleware), leaving the final handler unwrapped. | ||
| * | ||
| * For `method: 'ALL'` handlers that are last-for-group (from `.use()` or `.all()`), | ||
| * we use an arity (# of params) heuristic: middleware takes `(context, next)` (length >= 2), |
| getIsolationScope().setTransactionName(`${context.req.method} ${routePath(context)}`); | ||
|
|
||
| if (context.error) { | ||
| if (context.error && !isAlreadyCaptured(context.error)) { |
Member
There was a problem hiding this comment.
Maybe we can skip the check here as this is already handled in captureException
| function wrapSubAppMiddleware(routes: HonoRoute[]): void { | ||
| const lastIndexByKey = new Map<string, number>(); | ||
| for (const [i, route] of routes.entries()) { | ||
| lastIndexByKey.set(`${route.method}\0${route.path}`, i); |
Member
There was a problem hiding this comment.
Maybe add a comment why we need /0 in here
….all() handlers Hono registers both .use() middleware and .all() route handlers with method:'ALL' internally. Previously patchRoute wrapped all method:'ALL' handlers as middleware spans, incorrectly tracing .all() final handlers. Use handler.length >= 2 (accepts context + next) to identify middleware vs handler.length < 2 (accepts only context) for route handlers. Also guard responseHandler against re-capturing errors already captured by wrapMiddlewareWithSpan via isAlreadyCaptured check. Made-with: Cursor
…atterns Add comprehensive test coverage for the Hono middleware instrumentation: - E2E route-patterns tests for HTTP methods, registration styles, error capture - E2E inline middleware tests for direct method, .all(), .on() with inline and separately registered middleware across all HTTP methods - E2E negative test asserting .all() handlers don't create middleware spans - Unit tests for responseHandler error capture and isAlreadyCaptured guard - Unit tests for patchRoute arity heuristic (inline mw, separate mw, .on()) - Unit tests for sentry middleware without options (external init pattern) Made-with: Cursor
s1gr1d
force-pushed
the
sig/hono-route-pattern-tests
branch
from
1357397 to
fb1626e
Compare
47 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, Hono
.route()was patched by checking for an'ALL'handler (as those are used as middleware) to detect middleware in Hono sub-apps. But this incorrectly treated.all()final handlers as middleware. This broke error capture for.all()error routes (the wrapped handler caught and marked the error, preventing responseHandler from capturing it).Now we use an arity heuristic (arity = number of params): middleware accepts
(context, next), while route handlers accept only(context).Also added more middleware-related E2E test scenarios and route-patterns tests. Basically adding test scenarios from the Node integration test to the E2E test: https://github.com/getsentry/sentry-javascript/blob/5d0d14531511fcd703438e072723ef98cd700ea3/dev-packages/node-integration-tests/suites/tracing/hono/test.ts
Part of Project: #15260
Builds onto: #20449