fix(routing): match trailing-slash paths for Express compatibility#183
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/http/routing/route-registry.spec.ts (1)
100-100: ⚡ Quick winStrengthen this assertion to validate both HEAD paths, not just call count.
This prevents false positives if two calls happen for the wrong routes.
✅ Suggested assertion upgrade
expect(mockUwsApp.head).toHaveBeenCalledTimes(2); + expect(mockUwsApp.head).toHaveBeenNthCalledWith(1, '/items/:id', expect.any(Function)); + expect(mockUwsApp.head).toHaveBeenNthCalledWith(2, '/items/:id/', expect.any(Function));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/routing/route-registry.spec.ts` at line 100, Replace the loose call-count assertion on mockUwsApp.head with explicit assertions that the correct HEAD routes were registered: for each expected HEAD path used in this spec (e.g., the route path constants or the array/variable that lists HEAD routes), add expect(mockUwsApp.head).toHaveBeenCalledWith('<expectedPath>', expect.any(Function)) (or the equivalent matcher) for each path instead of only checking toHaveBeenCalledTimes(2); this ensures mockUwsApp.head was invoked with each specific route path and a handler function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http/routing/route-registry.ts`:
- Around line 425-433: The current companion registration only adds
trailing-slash variants for non-trailing uwsPath, so routes registered with a
trailing slash (uwsPath ending with '/') don't get a non-trailing companion;
update the logic in the route registry to also handle the reverse case: when
uwsPath.endsWith('/') && uwsPath !== '/', compute a nonTrailingPath (strip the
trailing slash), build a companion key using normalizedMethod and that
nonTrailingPath, check this.routes.has(companionKey) and if absent set
this.routes.set(companionKey, { ...routeInfo, trailingSlash: false }) and call
uwsMethodFn.call(this.uwsApp, nonTrailingPath, createHandler(companionKey));
keep existing behavior for the original branch so both directions are covered
and duplicates are avoided.
---
Nitpick comments:
In `@src/http/routing/route-registry.spec.ts`:
- Line 100: Replace the loose call-count assertion on mockUwsApp.head with
explicit assertions that the correct HEAD routes were registered: for each
expected HEAD path used in this spec (e.g., the route path constants or the
array/variable that lists HEAD routes), add
expect(mockUwsApp.head).toHaveBeenCalledWith('<expectedPath>',
expect.any(Function)) (or the equivalent matcher) for each path instead of only
checking toHaveBeenCalledTimes(2); this ensures mockUwsApp.head was invoked with
each specific route path and a handler function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8c161b0-4007-4a8c-ba6d-4b1d3aa10410
📒 Files selected for processing (3)
CHANGELOG.mdsrc/http/routing/route-registry.spec.tssrc/http/routing/route-registry.ts
5157d30 to
e69a298
Compare
uWestJS Benchmark Results
|
e69a298 to
4dcf02f
Compare
Auto-registers a trailing-slash variant for all simple routes (e.g.
/api→ also registers/api/). Derived routes are tagged withtrailingSlash: trueso they stay hidden fromgetRoutes()/getRouteCount(). Explicit HEAD overrides now also update the trailing-slash variant if one exists.uWebSockets.js uses exact-path routing, so
/api/returned 404 even though/apiworked. Express treats both as the same route by default.Fixes #182
Summary by CodeRabbit
Release Notes