-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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): fix source map remapping with multiple sources #18150
fix(ssr): fix source map remapping with multiple sources #18150
Conversation
Run & review this pull request in StackBlitz Codeflow. |
This reverts commit eea342d.
I thought I can revert #15805, but it's still broken for client build of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @hi-ogawa, Vite and Vitest are lucky to have you around! 💯
I've verified the fix with original issue. I also built you a test case that nicely verifies the fix, AriPerkkio@ef3433c. You can add it here with following:
$ git remote add ari https://github.com/AriPerkkio/vite.git
$ git fetch ari
$ git cherry-pick ef3433c005d37764f8868ff4dee730b4bf77cb06
That test has following results when the fix is applied:
expect(sources).toMatchInlineSnapshot(`
[
+ "nested-directory/nested-file.js",
+ "entrypoint.js",
- "nested-directory/nested-directory/nested-file.js",
- "nested-directory/entrypoint.js",
]
`)
We should also run ecosystem-ci here. Vitest coverage tests have some edge cases for source maps with multiple sources.
@AriPerkkio Thanks, I pushed your test case! Will try ecosystem ci. |
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ analogjs, astro, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, redwoodjs, storybook, sveltekit, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vitest, vuepress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The new code feels more correct to me. I was had a feeling that the old code is suspicious, but didn't find any actual case that breaks.
…ple sources Co-authored-by: Ari Perkkiö <ari.perkkio@gmail.com>
…urces (#18204) Co-authored-by: Ari Perkkiö <ari.perkkio@gmail.com>
Description
map.sources
paths incorrectly #18144I'm not too sure, but it feels like ssr transform should pass
magic-string
's source map directly for remapping sincecombineSourcemaps
should take care merging it with originalinMap.sources
. The initial code has been there for a long time since 6cb04faThis change makes
useArrayInterface = true
, so theremapping
goes through a simplified path and the issue won't happen, but maybe there's still something wrong withuseArrayInterface = false
case.vite/packages/vite/src/node/utils.ts
Lines 859 to 875 in 63a1251
For the test case, we can check the tests added in #17677 still works. I'm not sure what we can do to test
useArrayInterface = false
case though.vite/packages/vite/src/node/ssr/__tests__/ssrTransform.spec.ts
Line 423 in 63a1251
Here is how it looks on evanw.github.io/source-map-visualization and this is same as main.