feat: implicit return for single expressions using acorn parser
- Parse code with acorn to detect single expression statements - Auto-wrap expressions with 'return await (...)' for implicit return - Exclude side-effects: assignments, updates (x++), delete operator - Update skill.md examples to remove console.log (keep await) - Add unit tests for shouldAutoReturn function
This commit is contained in:
@@ -46,6 +46,7 @@
|
||||
"@hono/node-ws": "^1.2.0",
|
||||
"@modelcontextprotocol/sdk": "^1.25.2",
|
||||
"@xmorse/cac": "^6.0.7",
|
||||
"acorn": "^8.15.0",
|
||||
"async-sema": "^3.1.1",
|
||||
"diff": "^8.0.2",
|
||||
"dom-accessibility-api": "^0.7.1",
|
||||
|
||||
@@ -12,6 +12,7 @@ import util from 'node:util'
|
||||
import { createRequire } from 'node:module'
|
||||
import { fileURLToPath } from 'node:url'
|
||||
import vm from 'node:vm'
|
||||
import * as acorn from 'acorn'
|
||||
import { createSmartDiff } from './diff-utils.js'
|
||||
import { getCdpUrl } from './utils.js'
|
||||
import { waitForPageLoad, WaitForPageLoadOptions, WaitForPageLoadResult } from './wait-for-page-load.js'
|
||||
@@ -61,6 +62,61 @@ const usefulGlobals = {
|
||||
structuredClone,
|
||||
} as const
|
||||
|
||||
/**
|
||||
* Determines if code should be auto-wrapped with `return await (...)`.
|
||||
* Returns true for single expression statements that aren't assignments.
|
||||
*/
|
||||
export function shouldAutoReturn(code: string): boolean {
|
||||
try {
|
||||
const ast = acorn.parse(code, {
|
||||
ecmaVersion: 'latest',
|
||||
allowAwaitOutsideFunction: true,
|
||||
allowReturnOutsideFunction: true,
|
||||
sourceType: 'script',
|
||||
})
|
||||
|
||||
// Must be exactly one statement
|
||||
if (ast.body.length !== 1) {
|
||||
return false
|
||||
}
|
||||
|
||||
const stmt = ast.body[0]
|
||||
|
||||
// If it's already a return statement, don't auto-wrap
|
||||
if (stmt.type === 'ReturnStatement') {
|
||||
return false
|
||||
}
|
||||
|
||||
// Must be an ExpressionStatement
|
||||
if (stmt.type !== 'ExpressionStatement') {
|
||||
return false
|
||||
}
|
||||
|
||||
// Don't auto-return side-effect expressions
|
||||
const expr = stmt.expression
|
||||
if (
|
||||
expr.type === 'AssignmentExpression' ||
|
||||
expr.type === 'UpdateExpression' ||
|
||||
(expr.type === 'UnaryExpression' && (expr as acorn.UnaryExpression).operator === 'delete')
|
||||
) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Don't auto-return sequence expressions that contain assignments
|
||||
if (expr.type === 'SequenceExpression') {
|
||||
const hasAssignment = expr.expressions.some((e: acorn.Expression) => e.type === 'AssignmentExpression')
|
||||
if (hasAssignment) {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
return true
|
||||
} catch {
|
||||
// Parse failed, don't auto-return
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
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
|
||||
2. Clicked the extension icon on a tab to enable it (or refreshed the page if just installed)`
|
||||
@@ -721,8 +777,11 @@ export class PlaywrightExecutor {
|
||||
}
|
||||
|
||||
const vmContext = vm.createContext(vmContextObj)
|
||||
const wrappedCode = `(async () => { ${code} })()`
|
||||
const hasExplicitReturn = /\breturn\b/.test(code)
|
||||
const autoReturn = shouldAutoReturn(code)
|
||||
const wrappedCode = autoReturn
|
||||
? `(async () => { return await (${code}) })()`
|
||||
: `(async () => { ${code} })()`
|
||||
const hasExplicitReturn = autoReturn || /\breturn\b/.test(code)
|
||||
|
||||
const result = await Promise.race([
|
||||
vm.runInContext(wrappedCode, vmContext, { timeout, displayErrors: true }),
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import { shouldAutoReturn } from './executor.js'
|
||||
|
||||
describe('shouldAutoReturn', () => {
|
||||
it('returns true for simple expressions', () => {
|
||||
expect(shouldAutoReturn('1 + 2')).toBe(true)
|
||||
expect(shouldAutoReturn('page.title()')).toBe(true)
|
||||
expect(shouldAutoReturn('await page.title()')).toBe(true)
|
||||
expect(shouldAutoReturn('accessibilitySnapshot({ page })')).toBe(true)
|
||||
expect(shouldAutoReturn('context.pages().map(p => p.url())')).toBe(true)
|
||||
})
|
||||
|
||||
it('returns true for awaited expressions', () => {
|
||||
expect(shouldAutoReturn('await Promise.resolve(42)')).toBe(true)
|
||||
expect(shouldAutoReturn('await page.goto("https://example.com")')).toBe(true)
|
||||
})
|
||||
|
||||
it('returns false for assignment expressions', () => {
|
||||
expect(shouldAutoReturn('state.page = await context.newPage()')).toBe(false)
|
||||
expect(shouldAutoReturn('const x = 1')).toBe(false)
|
||||
expect(shouldAutoReturn('let y = 2')).toBe(false)
|
||||
expect(shouldAutoReturn('x = 5')).toBe(false)
|
||||
})
|
||||
|
||||
it('returns false for update expressions', () => {
|
||||
expect(shouldAutoReturn('x++')).toBe(false)
|
||||
expect(shouldAutoReturn('++x')).toBe(false)
|
||||
expect(shouldAutoReturn('x--')).toBe(false)
|
||||
expect(shouldAutoReturn('--x')).toBe(false)
|
||||
})
|
||||
|
||||
it('returns false for delete expressions', () => {
|
||||
expect(shouldAutoReturn('delete obj.prop')).toBe(false)
|
||||
})
|
||||
|
||||
it('returns false for multiple statements', () => {
|
||||
expect(shouldAutoReturn('const x = 1; x + 1')).toBe(false)
|
||||
expect(shouldAutoReturn('await page.click("button"); await page.title()')).toBe(false)
|
||||
expect(shouldAutoReturn('const a = 1\nconst b = 2')).toBe(false)
|
||||
})
|
||||
|
||||
it('returns false for return statements', () => {
|
||||
expect(shouldAutoReturn('return 5')).toBe(false)
|
||||
expect(shouldAutoReturn('return await page.title()')).toBe(false)
|
||||
})
|
||||
|
||||
it('returns false for non-expression statements', () => {
|
||||
expect(shouldAutoReturn('if (true) { x }')).toBe(false)
|
||||
expect(shouldAutoReturn('for (let i = 0; i < 10; i++) {}')).toBe(false)
|
||||
expect(shouldAutoReturn('function foo() {}')).toBe(false)
|
||||
expect(shouldAutoReturn('class Foo {}')).toBe(false)
|
||||
})
|
||||
|
||||
it('returns false for invalid syntax', () => {
|
||||
expect(shouldAutoReturn('const')).toBe(false)
|
||||
expect(shouldAutoReturn('{')).toBe(false)
|
||||
expect(shouldAutoReturn('await')).toBe(false)
|
||||
})
|
||||
|
||||
it('handles edge cases', () => {
|
||||
// Empty string
|
||||
expect(shouldAutoReturn('')).toBe(false)
|
||||
// Just whitespace
|
||||
expect(shouldAutoReturn(' ')).toBe(false)
|
||||
// Object literal (parsed as block)
|
||||
expect(shouldAutoReturn('{ foo: 1 }')).toBe(false)
|
||||
// Parenthesized object literal (expression)
|
||||
expect(shouldAutoReturn('({ foo: 1 })')).toBe(true)
|
||||
// Array literal
|
||||
expect(shouldAutoReturn('[1, 2, 3]')).toBe(true)
|
||||
})
|
||||
|
||||
it('handles sequence expressions with assignments', () => {
|
||||
// Sequence with assignment should not auto-return
|
||||
expect(shouldAutoReturn('(x = 1, x + 1)')).toBe(false)
|
||||
// Sequence without assignment should auto-return
|
||||
expect(shouldAutoReturn('(1, 2, 3)')).toBe(true)
|
||||
})
|
||||
|
||||
it('handles comments in code', () => {
|
||||
expect(shouldAutoReturn('// comment\npage.title()')).toBe(true)
|
||||
expect(shouldAutoReturn('page.title() // comment')).toBe(true)
|
||||
expect(shouldAutoReturn('/* block */ 42')).toBe(true)
|
||||
})
|
||||
|
||||
it('handles template literals', () => {
|
||||
expect(shouldAutoReturn('`hello`')).toBe(true)
|
||||
expect(shouldAutoReturn('`hello ${name}`')).toBe(true)
|
||||
})
|
||||
})
|
||||
@@ -63,13 +63,13 @@ playwriter -s 1 -e "state.page = await context.newPage(); await state.page.goto(
|
||||
playwriter -s 1 -e "await page.click('button')"
|
||||
|
||||
# Get page title
|
||||
playwriter -s 1 -e "console.log(await page.title())"
|
||||
playwriter -s 1 -e "await page.title()"
|
||||
|
||||
# Take a screenshot
|
||||
playwriter -s 1 -e "await page.screenshot({ path: 'screenshot.png', scale: 'css' })"
|
||||
|
||||
# Get accessibility snapshot
|
||||
playwriter -s 1 -e "console.log(await accessibilitySnapshot({ page }))"
|
||||
playwriter -s 1 -e "await accessibilitySnapshot({ page })"
|
||||
```
|
||||
|
||||
**Multiline code:**
|
||||
@@ -167,7 +167,7 @@ Locators (especially ones with `>> nth=`) can change when the page updates. Alwa
|
||||
await page.locator('[id="old-id"]').click(); // element may have changed
|
||||
|
||||
// GOOD: get fresh snapshot, then immediately use locators from it
|
||||
console.log(await accessibilitySnapshot({ page, showDiffSinceLastCall: true }));
|
||||
await accessibilitySnapshot({ page, showDiffSinceLastCall: true })
|
||||
// Now use the NEW locators from this output
|
||||
```
|
||||
|
||||
@@ -229,7 +229,7 @@ await loginPage.waitForURL('**/callback**');
|
||||
After any action (click, submit, navigate), verify what happened:
|
||||
|
||||
```js
|
||||
console.log('url:', page.url()); console.log(await accessibilitySnapshot({ page }));
|
||||
page.url() + '\n' + await accessibilitySnapshot({ page })
|
||||
```
|
||||
|
||||
For visually complex pages (grids, galleries, dashboards), use `screenshotWithAccessibilityLabels({ page })` instead to understand spatial layout. Label refs are short `eN` strings (e.g. `e3`).
|
||||
@@ -366,7 +366,7 @@ state.targetPage = pages[0];
|
||||
**List all available pages:**
|
||||
|
||||
```js
|
||||
console.log(context.pages().map(p => p.url()));
|
||||
context.pages().map(p => p.url())
|
||||
```
|
||||
|
||||
## navigation
|
||||
|
||||
Generated
+3
@@ -86,6 +86,9 @@ importers:
|
||||
'@xmorse/cac':
|
||||
specifier: ^6.0.7
|
||||
version: 6.0.7
|
||||
acorn:
|
||||
specifier: ^8.15.0
|
||||
version: 8.15.0
|
||||
async-sema:
|
||||
specifier: ^3.1.1
|
||||
version: 3.1.1
|
||||
|
||||
Reference in New Issue
Block a user