fix: use AST node offsets to strip trailing semicolons in execute wrapping
Single-expression code ending with `;` (e.g. `await foo();`) was wrapped as `return await (await foo();)` — the semicolon inside parens is a SyntaxError. Refactored `shouldAutoReturn` into `getAutoReturnExpression` which returns the exact expression source via acorn's `expr.start`/`expr.end` offsets, naturally excluding the trailing semicolon. More robust than regex stripping since it uses the parser we already have. Closes #58
This commit is contained in:
+36
-13
@@ -69,10 +69,11 @@ const usefulGlobals = {
|
|||||||
} as const
|
} as const
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Determines if code should be auto-wrapped with `return await (...)`.
|
* Parse code and check if it's a single expression that should be auto-returned.
|
||||||
* Returns true for single expression statements that aren't assignments.
|
* Returns the exact expression source (without trailing semicolon) using AST
|
||||||
|
* node offsets, or null if the code should not be auto-wrapped. See #58.
|
||||||
*/
|
*/
|
||||||
export function shouldAutoReturn(code: string): boolean {
|
export function getAutoReturnExpression(code: string): string | null {
|
||||||
try {
|
try {
|
||||||
const ast = acorn.parse(code, {
|
const ast = acorn.parse(code, {
|
||||||
ecmaVersion: 'latest',
|
ecmaVersion: 'latest',
|
||||||
@@ -83,19 +84,19 @@ export function shouldAutoReturn(code: string): boolean {
|
|||||||
|
|
||||||
// Must be exactly one statement
|
// Must be exactly one statement
|
||||||
if (ast.body.length !== 1) {
|
if (ast.body.length !== 1) {
|
||||||
return false
|
return null
|
||||||
}
|
}
|
||||||
|
|
||||||
const stmt = ast.body[0]
|
const stmt = ast.body[0]
|
||||||
|
|
||||||
// If it's already a return statement, don't auto-wrap
|
// If it's already a return statement, don't auto-wrap
|
||||||
if (stmt.type === 'ReturnStatement') {
|
if (stmt.type === 'ReturnStatement') {
|
||||||
return false
|
return null
|
||||||
}
|
}
|
||||||
|
|
||||||
// Must be an ExpressionStatement
|
// Must be an ExpressionStatement
|
||||||
if (stmt.type !== 'ExpressionStatement') {
|
if (stmt.type !== 'ExpressionStatement') {
|
||||||
return false
|
return null
|
||||||
}
|
}
|
||||||
|
|
||||||
// Don't auto-return side-effect expressions
|
// Don't auto-return side-effect expressions
|
||||||
@@ -105,24 +106,44 @@ export function shouldAutoReturn(code: string): boolean {
|
|||||||
expr.type === 'UpdateExpression' ||
|
expr.type === 'UpdateExpression' ||
|
||||||
(expr.type === 'UnaryExpression' && (expr as acorn.UnaryExpression).operator === 'delete')
|
(expr.type === 'UnaryExpression' && (expr as acorn.UnaryExpression).operator === 'delete')
|
||||||
) {
|
) {
|
||||||
return false
|
return null
|
||||||
}
|
}
|
||||||
|
|
||||||
// Don't auto-return sequence expressions that contain assignments
|
// Don't auto-return sequence expressions that contain assignments
|
||||||
if (expr.type === 'SequenceExpression') {
|
if (expr.type === 'SequenceExpression') {
|
||||||
const hasAssignment = expr.expressions.some((e: acorn.Expression) => e.type === 'AssignmentExpression')
|
const hasAssignment = expr.expressions.some((e: acorn.Expression) => e.type === 'AssignmentExpression')
|
||||||
if (hasAssignment) {
|
if (hasAssignment) {
|
||||||
return false
|
return null
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return true
|
// Use the expression node's start/end offsets to extract just the expression
|
||||||
|
// source, excluding any trailing semicolon. This is more robust than regex.
|
||||||
|
return code.slice(expr.start, expr.end)
|
||||||
} catch {
|
} catch {
|
||||||
// Parse failed, don't auto-return
|
// Parse failed, don't auto-return
|
||||||
return false
|
return null
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Backward-compatible helper: returns true if code should be auto-wrapped. */
|
||||||
|
export function shouldAutoReturn(code: string): boolean {
|
||||||
|
return getAutoReturnExpression(code) !== null
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Wraps user code in an async IIFE for vm execution.
|
||||||
|
* Uses AST node offsets to extract the expression without trailing semicolons,
|
||||||
|
* avoiding SyntaxError when embedding inside `return await (...)`. See #58.
|
||||||
|
*/
|
||||||
|
export function wrapCode(code: string): string {
|
||||||
|
const expr = getAutoReturnExpression(code)
|
||||||
|
if (expr !== null) {
|
||||||
|
return `(async () => { return await (${expr}) })()`
|
||||||
|
}
|
||||||
|
return `(async () => { ${code} })()`
|
||||||
|
}
|
||||||
|
|
||||||
const EXTENSION_NOT_CONNECTED_ERROR = `The Playwriter Chrome extension is not connected. Make sure you have:
|
const EXTENSION_NOT_CONNECTED_ERROR = `The Playwriter Chrome extension is not connected. Make sure you have:
|
||||||
1. Installed the extension: https://chromewebstore.google.com/detail/playwriter-mcp/jfeammnjpkecdekppnclgkkffahnhfhe
|
1. Installed the extension: https://chromewebstore.google.com/detail/playwriter-mcp/jfeammnjpkecdekppnclgkkffahnhfhe
|
||||||
2. Clicked the extension icon on a tab to enable it (or refreshed the page if just installed)`
|
2. Clicked the extension icon on a tab to enable it (or refreshed the page if just installed)`
|
||||||
@@ -1061,9 +1082,11 @@ export class PlaywrightExecutor {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const vmContext = vm.createContext(vmContextObj)
|
const vmContext = vm.createContext(vmContextObj)
|
||||||
const autoReturn = shouldAutoReturn(code)
|
const autoReturnExpr = getAutoReturnExpression(code)
|
||||||
const wrappedCode = autoReturn ? `(async () => { return await (${code}) })()` : `(async () => { ${code} })()`
|
const wrappedCode = autoReturnExpr !== null
|
||||||
const hasExplicitReturn = autoReturn || /\breturn\b/.test(code)
|
? `(async () => { return await (${autoReturnExpr}) })()`
|
||||||
|
: `(async () => { ${code} })()`
|
||||||
|
const hasExplicitReturn = autoReturnExpr !== null || /\breturn\b/.test(code)
|
||||||
|
|
||||||
// Track execution timestamps relative to recording start (seconds).
|
// Track execution timestamps relative to recording start (seconds).
|
||||||
// Used to identify idle gaps that can be sped up in demo videos.
|
// Used to identify idle gaps that can be sped up in demo videos.
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import { describe, it, expect } from 'vitest'
|
import { describe, it, expect } from 'vitest'
|
||||||
import { shouldAutoReturn } from './executor.js'
|
import { shouldAutoReturn, wrapCode } from './executor.js'
|
||||||
|
|
||||||
describe('shouldAutoReturn', () => {
|
describe('shouldAutoReturn', () => {
|
||||||
it('returns true for simple expressions', () => {
|
it('returns true for simple expressions', () => {
|
||||||
@@ -88,4 +88,50 @@ describe('shouldAutoReturn', () => {
|
|||||||
expect(shouldAutoReturn('`hello`')).toBe(true)
|
expect(shouldAutoReturn('`hello`')).toBe(true)
|
||||||
expect(shouldAutoReturn('`hello ${name}`')).toBe(true)
|
expect(shouldAutoReturn('`hello ${name}`')).toBe(true)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('returns true for expressions with trailing semicolons', () => {
|
||||||
|
expect(shouldAutoReturn('await page.title();')).toBe(true)
|
||||||
|
expect(shouldAutoReturn('page.title();')).toBe(true)
|
||||||
|
expect(shouldAutoReturn('snapshot({ page });')).toBe(true)
|
||||||
|
expect(shouldAutoReturn('await screenshotWithAccessibilityLabels({ page });')).toBe(true)
|
||||||
|
// trailing whitespace after semicolon
|
||||||
|
expect(shouldAutoReturn('await foo(); ')).toBe(true)
|
||||||
|
// comment after semicolon
|
||||||
|
expect(shouldAutoReturn('await foo(); // comment')).toBe(true)
|
||||||
|
// double semicolons are two statements, not auto-return
|
||||||
|
expect(shouldAutoReturn('await foo();;')).toBe(false)
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
describe('wrapCode', () => {
|
||||||
|
it('wraps single expressions with return await', () => {
|
||||||
|
expect(wrapCode('await page.title()')).toBe(
|
||||||
|
'(async () => { return await (await page.title()) })()',
|
||||||
|
)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('strips trailing semicolons to avoid SyntaxError (#58)', () => {
|
||||||
|
// The bug: trailing semicolons inside return await (...;) produce invalid JS
|
||||||
|
const wrapped = wrapCode('await screenshotWithAccessibilityLabels({ page });')
|
||||||
|
expect(wrapped).toBe(
|
||||||
|
'(async () => { return await (await screenshotWithAccessibilityLabels({ page })) })()',
|
||||||
|
)
|
||||||
|
// Verify it's valid JS by parsing it
|
||||||
|
expect(() => new Function(wrapped)).not.toThrow()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('produces valid JS for trailing semicolons on simple calls', () => {
|
||||||
|
const wrapped = wrapCode('page.title();')
|
||||||
|
expect(wrapped).toBe('(async () => { return await (page.title()) })()')
|
||||||
|
expect(() => new Function(wrapped)).not.toThrow()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('wraps multi-statement code without return', () => {
|
||||||
|
const wrapped = wrapCode('const x = 1; console.log(x);')
|
||||||
|
expect(wrapped).toBe('(async () => { const x = 1; console.log(x); })()')
|
||||||
|
})
|
||||||
|
|
||||||
|
it('wraps assignment expressions without return', () => {
|
||||||
|
expect(wrapCode('x = 5')).toBe('(async () => { x = 5 })()')
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
Reference in New Issue
Block a user