-
-
Notifications
You must be signed in to change notification settings - Fork 789
fix(request): return empty string for empty catch-all param #4395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fix: ensure undefined parameters are handled correctly in HonoRequest
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4395 +/- ##
=======================================
Coverage 91.25% 91.25%
=======================================
Files 171 171
Lines 10913 10913
Branches 3143 3144 +1
=======================================
Hits 9959 9959
Misses 953 953
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @amitksingh0880, thank you for creating the pull request! I believe the changes you made resolve the issue, but I have a few requests.
I think it would be good to make it like #4396. Could you please consider this? |
fix: ensure undefined parameters are handled correctly in HonoRequest
fix: ensure undefined parameters are handled correctly in HonoRequest
@amitksingh0880 Thank you for the PR! The followings fail:
I think these should not fail. For example, please imagine the app: app.get('/foo/:foo', (c) => {
const bar = c.req.param('bar')
return c.json({ bar })
}) When accessing So the test you wrote:
Are wrong. Those should be like the following: diff --git a/src/request.test.ts b/src/request.test.ts
index e04d4dfc..7a063deb 100644
--- a/src/request.test.ts
+++ b/src/request.test.ts
@@ -64,7 +64,7 @@ describe('Param', () => {
])
expect(req.param('id')).toBe('123')
- expect(req.param('name')).toBe('')
+ expect(req.param('name')).toBe(undefined)
req.routeIndex = 1
expect(req.param('id')).toBe('123')
@@ -81,7 +81,7 @@ describe('Param', () => {
])
expect(req.param('id')).toBe('123')
- expect(req.param('name')).toBe('')
+ expect(req.param('name')).toBe(undefined)
req.routeIndex = 1
expect(req.param('id')).toBe('456') @usualoma What do you think of this? |
Hey! @yusukebe thank you for the comment. I will look into the described changes and update the same. |
df3ed01
to
68510e6
Compare
Co-authored-by: Taku Amano <[email protected]>
I've updated the logic and tests myself. This may be good. @usualoma Can you review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@amitksingh0880 @usualoma I'll merge this now. Thank you! |
Fix: Ensure c.req.param() returns "" (empty string) for empty path parameters
Previously, when a route parameter matched an empty segment (e.g., / for /:remaining{.*}), c.req.param() returned undefined, which did not match its type signature (string).
This change updates the parameter extraction logic to return an empty string ("") instead of undefined for empty matches, ensuring runtime behavior matches the declared types and preventing potential runtime errors.
Also adds a test to verify this behavior.