Skip to content

Conversation

@hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Sep 20, 2024

Description

I'm not too sure, but it feels like ssr transform should pass magic-string's source map directly for remapping since combineSourcemaps should take care merging it with original inMap.sources. The initial code has been there for a long time since 6cb04fa

This change makes useArrayInterface = true, so the remapping goes through a simplified path and the issue won't happen, but maybe there's still something wrong with useArrayInterface = false case.

const useArrayInterface =
sourcemapList.slice(0, -1).find((m) => m.sources.length !== 1) === undefined
if (useArrayInterface) {
map = remapping(sourcemapList, () => null)
} else {
map = remapping(sourcemapList[0], function loader(sourcefile) {
const mapForSources = sourcemapList
.slice(mapIndex)
.find((s) => s.sources.includes(sourcefile))
if (mapForSources) {
mapIndex++
return mapForSources
}
return null
})
}


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.

test('sourcemap with multiple sources', async () => {

Here is how it looks on evanw.github.io/source-map-visualization and this is same as main.

@bolt-new-by-stackblitz
Copy link

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

@hi-ogawa
Copy link
Contributor Author

I thought I can revert #15805, but it's still broken for client build of with-define-object.ts though dev ssr case with-define-object-ssr.ts is fine.

@hi-ogawa hi-ogawa marked this pull request as ready for review September 20, 2024 04:19
AriPerkkio added a commit to AriPerkkio/vite that referenced this pull request Sep 20, 2024
Copy link
Contributor

@AriPerkkio AriPerkkio left a 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.

@hi-ogawa
Copy link
Contributor Author

@AriPerkkio Thanks, I pushed your test case! Will try ecosystem ci.

@hi-ogawa
Copy link
Contributor Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 9c29632: Open

suite result latest scheduled
histoire failure failure
remix failure failure
vike failure failure

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

@sapphi-red sapphi-red added feat: sourcemap Sourcemap support p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr labels Sep 26, 2024
Copy link
Member

@sapphi-red sapphi-red left a 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.

@patak-dev patak-dev merged commit e003a2c into vitejs:main Sep 26, 2024
hi-ogawa added a commit to hi-ogawa/vite that referenced this pull request Sep 26, 2024
@hi-ogawa hi-ogawa deleted the fix-ssr-multi-source-remapping branch September 30, 2024 05:24
sapphi-red pushed a commit that referenced this pull request Sep 30, 2024
moonlitusun pushed a commit to moonlitusun/vite that referenced this pull request May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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 this pull request may close these issues.

SSR transform sets map.sources paths incorrectly

4 participants