-
-
Notifications
You must be signed in to change notification settings - Fork 105
Fix: Gracefully handle syntax errors in definePage to prevent Vite server crashes #692
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
base: feat/experimental-router
Are you sure you want to change the base?
Changes from 4 commits
09c84cd
1923444
dded05e
5992675
d8052e0
f41937a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import { TransformResult } from 'vite' | ||
import { expect, describe, it } from 'vitest' | ||
import { definePageTransform, extractDefinePageNameAndPath } from './definePage' | ||
import { mockWarn } from '../../tests/vitest-mock-warn' | ||
|
||
const sampleCode = ` | ||
<script setup> | ||
|
@@ -18,6 +19,7 @@ | |
` | ||
|
||
describe('definePage', () => { | ||
mockWarn() | ||
it('removes definePage', async () => { | ||
const result = (await definePageTransform({ | ||
code: sampleCode, | ||
|
@@ -153,7 +155,7 @@ | |
expect(result?.code).toMatchSnapshot() | ||
}) | ||
|
||
it('throws if definePage uses a variable from the setup', async () => { | ||
it('handles definePage using a variable from setup gracefully', async () => { | ||
const code = ` | ||
<script setup> | ||
const a = 1 | ||
|
@@ -162,13 +164,13 @@ | |
}) | ||
</script> | ||
` | ||
// the function syntax works with sync and async errors | ||
await expect(async () => { | ||
await definePageTransform({ | ||
code, | ||
id: 'src/pages/basic.vue&definePage&vue', | ||
}) | ||
}).rejects.toThrowError() | ||
const result = await definePageTransform({ | ||
code, | ||
id: 'src/pages/basic.vue&definePage&vue', | ||
}) | ||
|
||
// Should return empty object instead of throwing | ||
expect(result).toBe('export default {}') | ||
}) | ||
|
||
it('extracts name and path', async () => { | ||
|
@@ -254,4 +256,86 @@ | |
path: '/custom', | ||
}) | ||
}) | ||
|
||
describe('error handling', () => { | ||
const codeWithSyntaxError = ` | ||
<script setup> | ||
definePage({ | ||
name: 'test',, // syntax error: extra comma | ||
path: '/test' | ||
}) | ||
</script> | ||
|
||
<template> | ||
<div>hello</div> | ||
</template> | ||
` | ||
|
||
it('handles syntax errors gracefully when extracting definePage', async () => { | ||
const result = await definePageTransform({ | ||
code: codeWithSyntaxError, | ||
id: 'src/pages/broken.vue?definePage&vue', | ||
}) | ||
|
||
// Should return empty object instead of crashing | ||
expect(result).toBe('export default {}') | ||
expect('Failed to process definePage:').toHaveBeenWarned() | ||
Check failure on line 282 in src/core/definePage.spec.ts
|
||
}) | ||
|
||
it('handles syntax errors gracefully when removing definePage from source', async () => { | ||
const codeForRemoval = ` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This piece of code is duplicated, no need to have it twice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the duplicated |
||
<script setup> | ||
const a = 1 | ||
definePage({ | ||
name: 'test',, // syntax error: extra comma | ||
path: '/test' | ||
}) | ||
const b = 1 | ||
Check failure on line 293 in src/core/definePage.spec.ts
|
||
</script> | ||
|
||
<template> | ||
<div>hello</div> | ||
</template> | ||
` | ||
|
||
const result = await definePageTransform({ | ||
code: codeForRemoval, | ||
id: 'src/pages/broken.vue', | ||
}) | ||
Check failure on line 304 in src/core/definePage.spec.ts
|
||
|
||
// Should return undefined (no transform) instead of crashing | ||
expect(result).toBeUndefined() | ||
expect('Failed to process definePage:').toHaveBeenWarned() | ||
Check failure on line 308 in src/core/definePage.spec.ts
|
||
}) | ||
|
||
it('handles extractDefinePageNameAndPath with syntax errors gracefully', async () => { | ||
const result = await extractDefinePageNameAndPath( | ||
codeWithSyntaxError, | ||
'src/pages/broken.vue' | ||
) | ||
|
||
// Should return null/undefined instead of crashing | ||
expect(result).toBeUndefined() | ||
expect('Failed to extract definePage info:').toHaveBeenWarned() | ||
Check failure on line 319 in src/core/definePage.spec.ts
|
||
}) | ||
|
||
it('handles unclosed brackets in definePage gracefully', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is just a syntax error, it's a redundant test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the redundant "handles unclosed brackets in definePage gracefully" test as it was just another syntax error variation already covered by existing tests. (d8052e0) |
||
const codeWithUnclosedBracket = ` | ||
<script setup> | ||
definePage({ | ||
name: 'test', | ||
path: '/test' | ||
// missing closing bracket | ||
</script> | ||
` | ||
|
||
const result = await definePageTransform({ | ||
code: codeWithUnclosedBracket, | ||
id: 'src/pages/unclosed.vue?definePage&vue', | ||
}) | ||
|
||
expect(result).toBe('export default {}') | ||
expect('Failed to process definePage:').toHaveBeenWarned() | ||
Check failure on line 338 in src/core/definePage.spec.ts
|
||
}) | ||
}) | ||
}) |
Uh oh!
There was an error while loading. Please reload this page.