-
-
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?
Fix: Gracefully handle syntax errors in definePage to prevent Vite server crashes #692
Conversation
Deploying unplugin-vue-router with
|
Latest commit: |
f41937a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://0b37225b.unplugin-vue-router.pages.dev |
Branch Preview URL: | https://copilot-fix-904c72ad-c6f9-42.unplugin-vue-router.pages.dev |
Co-authored-by: posva <[email protected]>
src/core/definePage.spec.ts
Outdated
|
||
describe('error handling', () => { | ||
it('handles syntax errors gracefully when extracting definePage', async () => { | ||
const codeWithSyntaxError = ` |
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.
this variable for a piece of code can be extracted in the outer scope. Share it among tests
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.
Extracted the codeWithSyntaxError variable to the outer scope of the error handling describe block to share it among the tests, avoiding duplication. (5992675)
src/core/definePage.spec.ts
Outdated
expect(result).toBeUndefined() | ||
}) | ||
|
||
it('handles malformed definePage object gracefully', async () => { |
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.
This test is redundant, syntax errors are already tested. Ensure you don't have duplicated testss
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.
Removed the redundant "handles malformed definePage object gracefully" test as it was duplicating syntax error testing already covered by other tests. (5992675)
src/core/definePage.spec.ts
Outdated
expect(result).toBe('export default {}') | ||
}) | ||
|
||
it('handles completely invalid JavaScript syntax gracefully', async () => { |
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.
same here, remove it, it's just syntax error
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.
Removed the redundant "handles completely invalid JavaScript syntax gracefully" test as it was just another syntax error variation already covered by existing tests. (5992675)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/experimental-router #692 +/- ##
===========================================================
Coverage ? 61.10%
===========================================================
Files ? 36
Lines ? 3409
Branches ? 626
===========================================================
Hits ? 2083
Misses ? 1319
Partials ? 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@copilot address the review |
…, remove redundant tests Co-authored-by: posva <[email protected]>
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.
src/core/definePage.spec.ts
Outdated
}) | ||
|
||
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the duplicated codeForRemoval
variable and now using the shared codeWithSyntaxError
variable to avoid duplication. (d8052e0)
src/core/definePage.spec.ts
Outdated
expect('Failed to extract definePage info:').toHaveBeenWarned() | ||
}) | ||
|
||
it('handles unclosed brackets in definePage gracefully', async () => { |
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.
this is just a syntax error, it's a redundant test
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.
Removed the redundant "handles unclosed brackets in definePage gracefully" test as it was just another syntax error variation already covered by existing tests. (d8052e0)
…fix syntax error warning handling Co-authored-by: posva <[email protected]>
Problem
Syntax errors in
definePage()
calls would crash the Vite development server, making it impossible to continue development when there were JavaScript syntax issues in the page definition. This created a poor developer experience where a simple typo could bring down the entire dev server.Solution
Enhanced error handling throughout the
definePage
processing pipeline to gracefully handle syntax errors and parsing failures:Key Changes
Added comprehensive try-catch blocks in
definePageTransform()
andextractDefinePageNameAndPath()
functions to catch any parsing or processing errorsEnhanced
getCodeAst()
function with error handling around Babel AST parsing to prevent syntax errors from propagatingChanged error behavior from throwing
SyntaxError
s to logging warnings and returning safe fallback values:'export default {}'
undefined
(no transform)Improved developer feedback by replacing crashes with helpful warning messages that identify the problematic file
Testing
Added comprehensive test coverage for various syntax error scenarios:
Impact
The fix ensures a more robust development experience while maintaining all existing functionality for properly formatted
definePage()
calls.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.