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

Jump to bottom

fix(error-logging): rollup errors weren't displaying id and codeframe #16540

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(error-logging): rollup errors weren't displaying id and codeframe #16540

wants to merge 1 commit into from
+47 −6

Conversation

thebanjomatic
Copy link
Contributor

@thebanjomatic thebanjomatic commented Apr 27, 2024 •
edited

Description

The code which modified the error message to have additional context about the module id and code frame where the error originated was relying on the error's stack having never been accessed prior to modifying the error's message. At some point, this no longer was true for Rollup errors, and no context information was being displayed as a result other than the stack trace within the rollup library that doesn't make it clear what or where the problem is.

This PR changes the code that add's the id and code-frame context to the error message to also modify the error's stack. To do so, it attempts to extract the original stack trace by removing the existing error message from error.stack if it matches, and otherwise falls back to just displaying the stack trace with its own message since it varies from the error.message.

Some additional normalization was done to the error.frame field if it exists to ensure that consistent padding is used in the error output. This is because the rollup code-frame doesn't have new-lines around it, but the esbuild one does.

Rollup Build Error Before:

error during build:
RollupError: Expression expected
    at getRollupError (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/parseAst.js:392:41)
    at ParseError.initialise (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:11170:28)
    at convertNode (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:12915:10)
    at convertProgram (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:12232:12)
    at Module.setSource (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:14076:24)
    at async ModuleLoader.addModuleSource (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:18729:13)

Rollup Build Error After:

error during build:
test.ts (9:0): Expression expected (Note that you need plugins to import files that are not JavaScript)
file: C:/Downloads/vitejs-vite-tjzm6p/test.ts:9:0

 7:
 8: // The following produces a rollup error
 9: @Component()
    ^
10: export default class Test extends Vue {
11:   bar: 1234;

RollupError: Expression expected
    at getRollupError (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/parseAst.js:392:41)
    at ParseError.initialise (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:11170:28)
    at convertNode (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:12915:10)
    at convertProgram (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:12232:12)
    at Module.setSource (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:14076:24)
    at async ModuleLoader.addModuleSource (file:///C:/Downloads/vitejs-vite-tjzm6p/node_modules/rollup/dist/es/shared/node-entry.js:18729:13)

The esbuild output remains unchanged, but should also be more consistently displayed even when the debugger is attached.

error during build:
Error: [vite:esbuild] Transform failed with 1 error:
C:/Downloads/vitejs-vite-tjzm6p/test.ts:5:6: ERROR: Expected identifier but found "import"
file: C:/Downloads/vitejs-vite-tjzm6p/test.ts:5:6

Expected identifier but found "import"
3  |
4  |  // The following produces an esbuild error
5  |  const import = 5;
   |        ^
6  |
7  |

    at failureErrorWithLog (C:\Downloads\vitejs-vite-tjzm6p\node_modules\esbuild\lib\main.js:1651:15)
    at C:\\Downloads\vitejs-vite-tjzm6p\node_modules\esbuild\lib\main.js:849:29
    at responseCallbacks.<computed> (C:\Downloads\vitejs-vite-tjzm6p\node_modules\esbuild\lib\main.js:704:9)      
    at handleIncomingPacket (C:\Downloads\vitejs-vite-tjzm6p\node_modules\esbuild\lib\main.js:764:9)
    at Socket.readFromStdout (C:\Downloads\vitejs-vite-tjzm6p\node_modules\esbuild\lib\main.js:680:7)
    at Socket.emit (node:events:517:28)
    at addChunk (node:internal/streams/readable:368:12)
    at readableAddChunk (node:internal/streams/readable:341:9)
    at Readable.push (node:internal/streams/readable:278:10)
    at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)

fixes #16539

@stackblitz StackBlitz
Copy link

stackblitz bot commented Apr 27, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@bluwy bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label Apr 29, 2024
@thebanjomatic
fix(error-logging): rollup errors weren't displaying id and codeframe
902b4eb 
The code which modified the error message to have additional context about the module id
and code frame where the error originated was relying on the error's stack having never
been accessed prior to modifying the error's message. At some point, this no longer was
true for Rollup errors, and no context information was being displayed as a result other
than the stack trace within the rollup library that doesn't make it clear what or where
the problem is.

This PR changes the code that add's the id and code-frame context to the error message
to also modify the error's stack. To do so, it attempts to extract the original stack
trace by removing the existing error message from error.stack if it matches, and
otherwise falls back to just displaying the stack trace with its own message since it
varies from the error.message.

Some additional normalization was done to the error.frame field if it exists to ensure
that consistent padding is used in the error output. This is because the rollup code-
frame doesn't have new-lines around it, but the esbuild one does.

Fixes vitejs#16539
@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 902b4eb: Open

suite result latest scheduled
nuxt ❌ failure ✅ success
sveltekit ❌ failure ❌ failure
vite-plugin-svelte ❌ failure ❌ failure
vitest ❌ failure ❌ failure

✅ analogjs, astro, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluwy bluwy bluwy approved these changes

Assignees
No one assigned
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error context from rollup / esbuild errors is swallowed and only the stack trace is printed
3 participants
@thebanjomatic @patak-dev @bluwy