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

Source maps missing for hoisted imports for ssrTransforms #16355

Closed
7 tasks done
smcenlly opened this issue Apr 5, 2024 · 3 comments · Fixed by #16356
Closed
7 tasks done

Source maps missing for hoisted imports for ssrTransforms #16355

smcenlly opened this issue Apr 5, 2024 · 3 comments · Fixed by #16356
Labels
feat: sourcemap Sourcemap support feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@smcenlly
Copy link
Contributor

smcenlly commented Apr 5, 2024

Describe the bug

We rely on source maps to correctly report errors to users in the dev tools that we create (Wallaby.js, Quokka.js, Console Ninja), but they are missing for import statements that are hoisted as a part of ssrTransforms.

See example source map visualization.

We've fixed the issue and created a test case against the main branch and will link to a PR once this issue has been created.

Validations

@hi-ogawa
Copy link
Collaborator

Interesting. Thanks for raising the issue and PR. It looks like it makes sense to preserve import statements if possible.

I'm not too familiar with this, but just looking at your sample source map, I wonder if it affects Vitest's coverage (probably in a good way). I'll ping a Vitest member just in case.

@AriPerkkio Do you know if this changes anything on Vitest coverage? With your recent PR vitest-dev/vitest#5423, it looks like coverage will ignore lines without source map, but this fix would maybe add them back, so it's a good thing?

@AriPerkkio
Copy link
Contributor

AriPerkkio commented Apr 10, 2024

Yes, code coverage (both V8 and Istanbul) consider all code that is present in source maps as user's source code. We do some exclusion for lines that are just causing noise already: https://github.com/vitest-dev/vitest/blob/6157282cc04494afc85085775d1f565c0d0dbb5a/packages/coverage-v8/src/provider.ts#L385-L390. We might need to add these patterns there as well.

I actually ran into this same issue in another Vitest related issue. Debuggers cannot stop on top of files that Vite SSR transform has transpiled due to missing source maps on import statements: vitest-dev/vitest#5355 (comment)

I tried to do similar fix as in #16356 (AriPerkkio@3707b9a#diff-cfc76e75093a8c9e1513ea8238e17e2cfa0fbb5751a0f3d0d10ab157fbfab6f3) but eventually ended patching Vite-node's source maps so that it will always work: https://github.com/vitest-dev/vitest/blob/6157282cc04494afc85085775d1f565c0d0dbb5a/packages/vite-node/src/source-map.ts#L51-L54.

@hi-ogawa hi-ogawa added p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr feat: sourcemap Sourcemap support and removed pending triage labels Apr 10, 2024
@hi-ogawa
Copy link
Collaborator

@AriPerkkio Thanks for chiming in! It's good to know you have investigated before.

I suppose fixing this would help ecosystem in general. I wonder if there are other heavy source map consumers other than Vitest and the OP of this issue. Depending on that, Vite team can maybe prioritize this.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: sourcemap Sourcemap support feat: ssr 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.

3 participants