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.

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

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Nov 11, 2024

Description

(Recreated #18621 as I force-pushed the branch and Github won't let me reopen it)

It looks like this parse error handling code is obsolete and does nothing for current rollup parse error. For non-js file, esbuild transform in plugin pipeline normally catches a syntax error, but for js file, ssrTransform needs to throw an error. Since ssrTransform is outside of transform plugin pipeline, Error.pos doesn't get prettified automatically, so I added a error processing inside the try/catch.

reproduction: https://stackblitz.com/edit/vitest-dev-vitest-wpj5mg?file=vite.config.ts

Screenshots
  • v5

image

  • this PR

image

@hi-ogawa hi-ogawa changed the title Fix format ssr transform parse error fix(ssr): format ssrTransform parse error Nov 11, 2024
@hi-ogawa hi-ogawa changed the title fix(ssr): format ssrTransform parse error fix(ssr): format ssrTransform parse error Nov 11, 2024
@hi-ogawa hi-ogawa marked this pull request as ready for review November 11, 2024 23:13
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.

@sapphi-red sapphi-red added feat: ssr p2-nice-to-have Not breaking anything but nice to have (priority) labels Nov 12, 2024
sapphi-red
sapphi-red previously approved these changes Nov 12, 2024
@sapphi-red sapphi-red added p2-edge-case Bug, but has workaround or limited in scope (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Nov 12, 2024
bluwy
bluwy previously approved these changes Nov 12, 2024
packages/vite/src/node/ssr/ssrTransform.ts Outdated Show resolved Hide resolved
@hi-ogawa hi-ogawa dismissed stale reviews from bluwy and sapphi-red via 2bc211a November 12, 2024 10:15
hi-ogawa and others added 2 commits November 12, 2024 19:15
Co-authored-by: Bjorn Lu <[email protected]>
@sapphi-red sapphi-red merged commit d9be921 into vitejs:main Nov 14, 2024
15 checks passed
@hi-ogawa hi-ogawa deleted the fix-format-ssr-transform-parse-error branch November 14, 2024 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log LOC for js syntax errors
3 participants