fix(http-router): resolve Hono method typing for Node 22 compatibility #37804
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughA TypeScript-only change in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
Jaysinh Patankar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Jaysinh146
force-pushed
the
fix/37801-node-22-typing
branch
from
2871a3e to
8bf0bab
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/http-router/src/Router.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/http-router/src/Router.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
packages/http-router/src/Router.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/http-router/src/Router.ts (1)
197-197: Avoidas Function; cast to a callable route-method type instead (keeps safety without changing runtime).
Right nowkeyof typeof this.innerRouteris too broad andas Functiondisables call-signature checking; a narrower callable type still fixes TS2349 while preserving intent.- (this.innerRouter[method.toLowerCase() as keyof typeof this.innerRouter] as Function)(`/${subpath}`.replace('//', '/'), ...middlewares, async (c: Context) => { + type RouteMethod = (path: string, ...handlers: MiddlewareHandler[]) => unknown; + const methodKey = method.toLowerCase() as Lowercase<Method>; + (this.innerRouter as unknown as Record<Lowercase<Method>, RouteMethod>)[methodKey]( + `/${subpath}`.replace('//', '/'), + ...middlewares, + async (c: Context) => { const { req, res } = c;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/http-router/src/Router.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/http-router/src/Router.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
packages/http-router/src/Router.ts
2 participants
Proposed changes
This PR resolves a TypeScript build failure occurring in
packages/http-routerwhen using Node 22 / TypeScript 5.x.Technical Changes:
method.toLowerCase()tokeyof typeof this.innerRouter. This fixes the issue where TS could not verify the string was a valid router key.Function. This prevents theTS2349: Expression is not callableerror by assuring TS that we are accessing a method, not a property array (like.routes).(c)asanyto resolve theTS7006: Implicit anyerror.These changes allow the project to build successfully on the latest Node versions while preserving the existing runtime logic.
Issue(s)
Closes #37801
Steps to test or reproduce
developbranch and runyarn buildinpackages/http-router. Observe the TypeScript errors (TS2345, TS7006).yarn buildinpackages/http-router.Further comments
Thanks to @17dhruv for identifying the root cause and providing the reproduction steps in the issue.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.