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

Subtle change in Vite runtime stacktrace with prepareStackTrace since 5.1.5 #16178

Closed
7 tasks done
hi-ogawa opened this issue Mar 16, 2024 · 2 comments · Fixed by #16220
Closed
7 tasks done

Subtle change in Vite runtime stacktrace with prepareStackTrace since 5.1.5 #16178

hi-ogawa opened this issue Mar 16, 2024 · 2 comments · Fixed by #16220
Labels
feat: sourcemap Sourcemap support p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release

Comments

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Mar 16, 2024

Describe the bug

While upgrading Vite from 5.1.0 to 5.1.6 on my monorepo with vite-node-miniflare hi-ogawa/vite-plugins#198, I found my tests which asserts stacktrace are failing. On further investigation, there seems to be a subtle difference since 5.1.5 as seen in this simpler reproduction.

node run.mjs ./repro-entry.ts
...
Error: crash ssr
    at crash (.../repro-entry.ts:2:9)
    at main (.../repro-entry.ts:6:9)  <---- 6:3 became 6:9 since 5.1.5
function crash(message) {
  throw new Error(message);
}

function main() {
  crash("crash ssr");
//^ 6:3        <- before
//      ^ 6:9  <- after
}

main();

On my actual case, I use a plugin vitePluginSimpleHmr which adds one more transform to ssr code, then stacktrace seems more broken (not only column, but also line and absolute path resolution is not working). For this part, there might be an issue with my plugin, so I'm not sure whether it's related to Vite side change and I haven't tried minimal repro for that yet.

Reproduction

https://github.com/hi-ogawa/reproductions/tree/main/vite-runtime-stacktrace-5.1.5

Steps to reproduce

In the reproduction, I added a comparison with node and also when vite entry is plain javascript, in which case there's no difference in stacktrace.

Stackblitz doesn't seem to have a good support on stacktrace, so it might be necessary to copy the reproduction locally. To skip cloning the entire repo, you can use tools such as https://github.com/tiged/tiged

npx tiged hi-ogawa/reproductions/vite-runtime-stacktrace-5.1.5 repro

System Info

  System:
    OS: Linux 6.7 Arch Linux
    CPU: (12) x64 AMD Ryzen 5 5625U with Radeon Graphics
    Memory: 9.30 GB / 14.98 GB
    Container: Yes
    Shell: 5.2.26 - /bin/bash
  Binaries:
    Node: 20.11.1 - ~/.volta/tools/image/node/20.11.1/bin/node
    Yarn: 1.22.19 - ~/.volta/tools/image/yarn/1.22.19/bin/yarn
    npm: 10.2.4 - ~/.volta/tools/image/node/20.11.1/bin/npm
    pnpm: 8.12.0 - ~/.volta/bin/pnpm
    bun: 1.0.26 - ~/.volta/bin/bun
  Browsers:
    Chromium: 122.0.6261.111
  npmPackages:
    vite: 5.1.5 => 5.1.5

Used Package Manager

npm

Logs

This is the same content as reproduction readme, but just in case:

# node
node ./repro-entry.mjs
...
Error: crash ssr
    at crash (.../repro-entry.mjs:2:9)
    at main (.../repro-entry.mjs:6:3)
    ...


# vite + js
node run.mjs ./repro-entry.mjs
...
Error: crash ssr
    at crash (.../repro-entry.mjs:2:9)
    at main (.../repro-entry.mjs:6:3)
    ...


# vite 5.1.4 + ts
node run.mjs ./repro-entry.ts
...
Error: crash ssr
    at crash (.../repro-entry.ts:2:9)
    at main (.../repro-entry.ts:6:3)
    ...


# vite 5.1.5 + ts
node run.mjs ./repro-entry.ts
...
Error: crash ssr
    at crash (.../repro-entry.ts:2:9)
    at main (.../repro-entry.ts:6:9)  <---- 6:3 became 6:9

Validations

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release feat: sourcemap Sourcemap support labels Mar 16, 2024
@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Mar 21, 2024

I did some digging in #16220 and it seems bias === LEAST_UPPER_BOUND => true transform introduced in #15907 doesn't seem to be correct.

For example, when solving originalPositionFor for { line: 7, column: 2 }) and this sourcemap, I see a following combination in traceSegmentInternal

  • found: false
  • index: 0
  • bias: 1 (default from GREATEST_LOWER_BOUND = 1)

Then, due to bias === LEAST_UPPER_BOUND (= -1) replaced to true, index: 0 is pushed to index: 1, so it's picking up a next segment in originalPositionFor (i.e. 6:9 instead of 6:3 as in the reproduction).


Also this transform to help tree-shaking might not be necessary anymore since @jridgewell/trace-mapping did some restructuring in jridgewell/trace-mapping@fc2a76a

I tested removing "replace bias" transform in #16220 and the bundle size of dist/runtime.js seems to be mostly same.

@sapphi-red Do you think it's okay to remove this transform now? Also if you have some insight on bias === LEAST_UPPER_BOUND, I would be happy to hear.

@sapphi-red
Copy link
Member

I added the replacement so that I can preserve the code before the PR.
The code before:

let index = memoizedBinarySearch(segments, column, memo, line)
if (found) {
index = lowerBound(segments, column, index)
}
if (index === -1 || index === segments.length) return -1
return index

The code in trace-mapping:
https://github.com/jridgewell/trace-mapping/blob/2c7bf531d923eb304a4b647148baecd4aae1130f/src/trace-mapping.ts#L381-L387
But actually bias === LEAST_UPPER_BOUND should be replaced with false instead of true🤦.

I'm fine with removing the replacement as long as the runtime chunk doesn't get that bigger because I guess that was the intent.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: sourcemap Sourcemap support p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants