Skip to content
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.

Sign up for GitHub

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

fix(ssr): format ssrTransform parse error #18644

Merged
merged 10 commits into from
Nov 14, 2024
Merged

fix(ssr): format ssrTransform parse error #18644

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
71ec17c
fix(ssr): format ssrTransform parse error
hi-ogawa Nov 8, 2024
a8c7905
test: add test
hi-ogawa Nov 8, 2024
5d30090
chore: replace test
hi-ogawa Nov 9, 2024
fb6ab11
fix: err.id too
hi-ogawa Nov 9, 2024
77ca432
chore: tweak message
hi-ogawa Nov 12, 2024
fce167f
fix: err.pos might be missing
hi-ogawa Nov 12, 2024
514a0de
chore: tweak
hi-ogawa Nov 12, 2024
2bc211a
chore: typo
hi-ogawa Nov 12, 2024
e33f473
chore: f
hi-ogawa Nov 12, 2024
42eec0c
chore: f
hi-ogawa Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions packages/vite/src/node/ssr/__tests__/__snapshots__/ssrLoadModule.spec.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,15 @@ Expected ";" but found "code"

exports[`parse error 2`] = `
{
"frame": "",
"id": "",
"loc": undefined,
"frame": "1 | invalid code
| ^
2 | ",
"id": "/fixtures/errors/syntax-error.js",
"loc": {
"column": 9,
"file": "/fixtures/errors/syntax-error.js",
"line": 1,
},
"message": "Expected ';', '}' or <eof>",
}
`;
Expand All @@ -49,9 +55,15 @@ Expected ";" but found "code"

exports[`parse error 4`] = `
{
"frame": "",
"id": "",
"loc": undefined,
"frame": "1 | invalid code
| ^
2 | ",
"id": "/fixtures/errors/syntax-error.js",
"loc": {
"column": 9,
"file": "/fixtures/errors/syntax-error.js",
"line": 1,
},
"message": "Expected ';', '}' or <eof>",
}
`;
23 changes: 13 additions & 10 deletions packages/vite/src/node/ssr/ssrTransform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ import { walk as eswalk } from 'estree-walker'
import type { RawSourceMap } from '@ampproject/remapping'
import { parseAstAsync as rollupParseAstAsync } from 'rollup/parseAst'
import type { TransformResult } from '../server/transformRequest'
import { combineSourcemaps, isDefined } from '../utils'
import {
combineSourcemaps,
generateCodeFrame,
isDefined,
numberToPos,
} from '../utils'
import { isJSONRequest } from '../plugins/json'
import type { DefineImportMetadata } from '../../shared/ssrTransform'

Expand Down Expand Up @@ -81,15 +86,13 @@ async function ssrTransformScript(
try {
ast = await rollupParseAstAsync(code)
} catch (err) {
if (!err.loc || !err.loc.line) throw err
const line = err.loc.line
throw new Error(
`Parse failure: ${
err.message
}\nAt file: ${url}\nContents of line ${line}: ${
code.split('\n')[line - 1]
}`,
)
if (err.code === 'PARSE_ERROR' && typeof err.pos === 'number') {
err.id = url
err.loc = numberToPos(code, err.pos)
err.loc.file = url
err.frame = generateCodeFrame(code, err.pos)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the error message (Parse failure: ...) so that console.log(err) shows the position and contents (#5192, #12060).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know when err.loc exsits (when it passes the previous code path) though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it looks better to keep Parse failure: ... for clarity. I remembered esbuild transform error also includes location details in its message (cf. #18626).

I don't know when err.loc exsits (when it passes the previous code path) though.

Probably loc check was for acorn based parser error, so it's no longer relevant. Since Rollup 4, it now only has code, message, pos properties https://github.com/rollup/rollup/blob/42e587e0e37bc0661aa39fe7ad6f1d7fd33f825c/src/utils/bufferToAst.ts#L20-L22, so we only need to target these. It looks like pos is missing on rust side panic, so I covered that case too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking! That makes sense.

}
throw err
}

let uid = 0
Expand Down
Loading