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

Error context from rollup / esbuild errors is swallowed and only the stack trace is printed #16539

Open
7 tasks done
thebanjomatic opened this issue Apr 26, 2024 · 1 comment · May be fixed by #16540
Open
7 tasks done
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@thebanjomatic
Copy link
Contributor

thebanjomatic commented Apr 26, 2024

Describe the bug

When rollup produce errors during build, the error message that is displayed does not always contain the relevant information such as which file produced the error, the code frame, or location information.

The esbuild error output appears more consistent, but as an example, an esbuild error produces the following output where the error.message is modified to include additional context like the file or the code frame where the error occurred:

Current EsBuild Output:

x Build failed in 111ms
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)

When a rollup error occurs, instead all we see is the following which is extremely unhelpful:
Current Rollup Error Output:

x Build failed in 110ms
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)

Interestingly the same code is being applied in both places and the formatting should be consistent between the two. What appears to be happening is that code that is logging the errors to the CLI is just using ${error.stack} to do so, and the stack property of an error is lazy generated the first time it is accessed. The vite error logging code is currently relying on this behavior when modifying the e.message with the expectation that the stack that is generated will have the correct message is appended to the top when displayed.

This assumption breaks in a couple different ways:

  1. Within the Rollup library internals, the stack of the generated error is accessed and read which causes the stack string to be hydrated with the original message. By doing this, modifying the message after the fact has no effect on the stack property and vite's error display breaks.

  2. When debugging a build failure with the debugger attached, the error object will have its properties queried including the stack to display in the locals field. When you resume code execution, the displayed error message for both esbuild and rollup will be lacking the contextual information.

Reproduction

https://stackblitz.com/edit/vitejs-vite-tjzm6p?file=test.ts

Steps to reproduce

run npm install followed by npm run build

You can uncomment the following line to see the esbuild error behavior:

//const import = 5;

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12800H
    Memory: 10.81 GB / 31.64 GB
  Binaries:
    Node: 18.20.2 - ~\AppData\Local\Volta\tools\image\node\18.20.2\node.EXE
    Yarn: 1.22.19 - ~\AppData\Local\Volta\tools\image\yarn\1.22.19\bin\yarn.CMD
    npm: 10.5.0 - ~\AppData\Local\Volta\tools\image\node\18.20.2\npm.CMD
    pnpm: 8.14.1 - ~\AppData\Local\Volta\tools\image\pnpm\8.14.1\bin\pnpm.CMD
    bun: 1.1.4 - ~\.bun\bin\bun.EXE
  Browsers:
    Edge: Chromium (123.0.2420.81)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    vite: ~5.2.10 => 5.2.10

Used Package Manager

npm

Logs

No response

Validations

Copy link

stackblitz bot commented Apr 26, 2024

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

thebanjomatic added a commit to thebanjomatic/vite that referenced this issue Apr 26, 2024
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 falling back to looking for the stack trace portion by searching for indented
lines that look something like:
    at (file://path/to/file.js
or
    at <annonymout>

Some additional normalization was done to the error.frame field if it exists to ensure
that consistent padding is used in the error output.

Fixes vitejs#16539
thebanjomatic added a commit to thebanjomatic/vite that referenced this issue Apr 27, 2024
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
thebanjomatic added a commit to thebanjomatic/vite that referenced this issue Apr 27, 2024
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
@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Apr 29, 2024
thebanjomatic added a commit to thebanjomatic/vite that referenced this issue Apr 29, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants