fix: handle minified arrow functions in hasReturn detection #1623
Conversation
The `hasReturn` function used hardcoded character positions to detect
arrow function expressions, assuming the format `) => expr`. This broke
when code was minified to `)=>expr` (no spaces), causing beforeHandle
and other lifecycle hooks to be incorrectly removed during AOT compilation.
The fix:
- Use `indexOf('=>')` to find the arrow operator regardless of spacing
- Skip whitespace after `=>` before checking for block braces
- Correctly identify direct return expressions in both minified and
unminified code
Fixes elysiajs#1617
WalkthroughhasReturn was changed to coerce non-object inputs to string via toString() and to detect direct-arrow-function returns by searching for the arrow token (=>) then checking the first non-whitespace character after it; tests for many arrow-function forms in beforeHandle hooks were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![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 (2)
src/compose.ts (1)
409-432: Consider supporting tabs and newlines after arrow operator.The whitespace-skipping logic only checks for spaces (char code 32) but not tabs (9) or newlines (10, 13). While minified code typically uses spaces, formatted code with tabs or newlines after
=>would not be detected correctly.Apply this diff to handle all whitespace characters:
// Check for arrow function expression (direct return without braces) // Handle both `) => x` and `)=>x` formats (with or without spaces) const arrowIndex = fnLiteral.indexOf('=>', parenthesisEnd) if (arrowIndex !== -1) { - // Skip any whitespace after `=>` + // Skip any whitespace after `=>` (space, tab, newline, carriage return) let afterArrow = arrowIndex + 2 while ( afterArrow < fnLiteral.length && - fnLiteral.charCodeAt(afterArrow) === 32 + (fnLiteral.charCodeAt(afterArrow) === 32 || // space + fnLiteral.charCodeAt(afterArrow) === 9 || // tab + fnLiteral.charCodeAt(afterArrow) === 10 || // newline + fnLiteral.charCodeAt(afterArrow) === 13) // carriage return ) { afterArrow++ }Alternatively, use a regex-based approach:
const arrowIndex = fnLiteral.indexOf('=>', parenthesisEnd) if (arrowIndex !== -1) { - // Skip any whitespace after `=>` - let afterArrow = arrowIndex + 2 - while ( - afterArrow < fnLiteral.length && - fnLiteral.charCodeAt(afterArrow) === 32 - ) { - afterArrow++ - } + // Skip any whitespace after `=>` using regex + const afterArrowMatch = fnLiteral.slice(arrowIndex + 2).match(/^\s*/) + const afterArrow = arrowIndex + 2 + (afterArrowMatch ? afterArrowMatch[0].length : 0) // If the first non-whitespace char after `=>` is not `{`, it's a direct return if (test/core/before-handle-arrow.test.ts (1)
1-132: Add explicit test for minified arrow function format.While the test suite is comprehensive, it would benefit from an explicit test case that reproduces the exact minified format mentioned in the issue:
()=>exprwithout spaces. This would serve as a regression test for the specific bug that was fixed.Add this test case:
it('should handle minified arrow function without spaces', async () => { let beforeHandleCalled = false // Simulate minified code: ()=>expr with no spaces const minifiedBeforeHandle = new Function( 'beforeHandleCalled', 'return ()=>(beforeHandleCalled.value=true,undefined)' )({ value: false }) const app = new Elysia() .get('/test', () => 'ok', { beforeHandle: minifiedBeforeHandle }) const response = await app.handle(new Request('http://localhost/test')) expect(response.status).toBe(200) // Note: This test structure needs adjustment to check the flag properly // as the closure won't work as written above. Consider using context instead. })Or simpler, using the context pattern from line 62:
it('should handle minified arrow expression format', async () => { const state = { called: false } const app = new Elysia() .get('/test', () => 'ok', { // Minified format without spaces: ()=>expr beforeHandle: ()=>(state.called=true) }) const response = await app.handle(new Request('http://localhost/test')) expect(response.status).toBe(200) expect(state.called).toBe(true) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/compose.ts(1 hunks)test/core/before-handle-arrow.test.ts(1 hunks)
🔇 Additional comments (2)
src/compose.ts (1)
405-405: LGTM: String coercion is correct.The change to call
v.toString()for the non-object, non-string branch is correct—the function needs the string representation to analyze arrow function syntax.test/core/before-handle-arrow.test.ts (1)
5-132: LGTM: Comprehensive test coverage for arrow function variants.The test suite thoroughly validates the fix across multiple scenarios: sync/async arrows, early returns, complex expressions, multiple hooks, and live server integration. This provides strong regression protection.
- Support tab, newline, and carriage return in whitespace skip logic - Add regression test with truly minified arrow function (no spaces) - Clean up unused variables in test file
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/compose.ts (2)
401-406: SimplifyfnLiteralcoercion (redundant ternary).Both branches call
toString(), so thetypeof v === 'string'check no longer does anything.- const fnLiteral = isObject - ? v.fn.toString() - : typeof v === 'string' - ? v.toString() - : v.toString() + const fnLiteral = isObject ? v.fn.toString() : v.toString()
407-436: Tighten arrow detection to avoid matching=>inside non-arrow function bodies.As-is, any
=>after the first)(including nested callbacks) can fliphasReturntotrue. This is usually benign, but you can cheaply reduce false positives by ensuring the=>appears before the first{(i.e., in the “signature” region).const parenthesisEnd = fnLiteral.indexOf(')') + const scanStart = parenthesisEnd === -1 ? 0 : parenthesisEnd + const braceIndex = fnLiteral.indexOf('{', scanStart) // Check for arrow function expression (direct return without braces) // Handle both `) => x` and `)=>x` formats (with or without spaces) - const arrowIndex = fnLiteral.indexOf('=>', parenthesisEnd) + const arrowIndex = fnLiteral.indexOf('=>', scanStart) - if (arrowIndex !== -1) { + if (arrowIndex !== -1 && (braceIndex === -1 || arrowIndex < braceIndex)) { // Skip any whitespace after `=>` (space, tab, newline, carriage return) let afterArrow = arrowIndex + 2 let charCode: number while ( afterArrow < fnLiteral.length && ((charCode = fnLiteral.charCodeAt(afterArrow)) === 32 || // space charCode === 9 || // tab charCode === 10 || // newline charCode === 13) // carriage return ) { afterArrow++ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/compose.ts(1 hunks)test/core/before-handle-arrow.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/core/before-handle-arrow.test.ts
1 participant
Summary
hasReturnfunction to correctly detect arrow function expressions in minified codebeforeHandlewith various arrow function formatsProblem
The
hasReturnfunction incompose.tsused hardcoded character positions to detect arrow function expressions:This broke when code was minified (e.g., by tsx) to
)=>expr(no spaces), because:) => expr: position +2 is=✓)=>expr: position +2 is>✗This caused
beforeHandleand other lifecycle hooks to be incorrectly identified as "not having a return", leading to them being silently removed during AOT compilation.Solution
Use
indexOf('=>')to find the arrow operator regardless of spacing:Test Plan
test/core/before-handle-arrow.test.tswith 7 test casesFixes #1617
Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.