fix(ssr): fix source map remapping with multiple sources by hi-ogawa · Pull Request #18150 · vitejs/vite

hi-ogawa · 2024-09-20T02:51:49Z

Description

closes SSR transform sets map.sources paths incorrectly #18144

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.

vite/packages/vite/src/node/utils.ts

Lines 859 to 875 in 63a1251

    
           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.

vite/packages/vite/src/node/ssr/__tests__/ssrTransform.spec.ts

Line 423 in 63a1251

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 · 2024-09-20T02:51:51Z

Run & review this pull request in StackBlitz Codeflow.

This reverts commit eea342d.

hi-ogawa · 2024-09-20T04:18:04Z

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.

AriPerkkio

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 · 2024-09-20T07:26:34Z

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

hi-ogawa · 2024-09-20T07:26:39Z

/ecosystem-ci run

vite-ecosystem-ci · 2024-09-20T07:42:09Z

📝 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

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ö <[email protected]>

…urces (#18204) Co-authored-by: Ari Perkkiö <[email protected]>

Co-authored-by: Ari Perkkiö <[email protected]>

fix(ssr): fix source map remapping with multiple sources

63a1251

hi-ogawa added 2 commits September 20, 2024 13:07

refactor: can revert vitejs#15805 ?

eea342d

Revert "refactor: can revert vitejs#15805 ?"

481e7b4

This reverts commit eea342d.

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

test: add test for vitejs#18150

ef3433c

AriPerkkio approved these changes Sep 20, 2024

View reviewed changes

test: add test for vitejs#18150

9c29632

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

sapphi-red approved these changes Sep 26, 2024

View reviewed changes

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

fix(ssr): (backport vitejs#18150) fix source map remapping with multi…

e47b220

…ple sources Co-authored-by: Ari Perkkiö <[email protected]>

hi-ogawa mentioned this pull request Sep 26, 2024

fix(ssr): (backport #18150) fix source map remapping with multiple sources #18204

Merged

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

fix(ssr): (backport #18150) fix source map remapping with multiple so…

262a879

…urces (#18204) Co-authored-by: Ari Perkkiö <[email protected]>

hi-ogawa mentioned this pull request Mar 6, 2025

fix(sourcemap): combine sourcemaps with multiple sources without matched source #18971

Merged

moonlitusun pushed a commit to moonlitusun/vite that referenced this pull request May 25, 2025

fix(ssr): fix source map remapping with multiple sources (vitejs#18150)

d9baeb1

Co-authored-by: Ari Perkkiö <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

fix(ssr): fix source map remapping with multiple sources#18150

fix(ssr): fix source map remapping with multiple sources#18150
patak-dev merged 4 commits intovitejs:mainfrom
hi-ogawa:fix-ssr-multi-source-remapping

hi-ogawa commented Sep 20, 2024 •

edited

Loading

Uh oh!

bolt-new-by-stackblitz bot commented Sep 20, 2024

Uh oh!

hi-ogawa commented Sep 20, 2024

Uh oh!

AriPerkkio left a comment •

edited

Loading

Uh oh!

hi-ogawa commented Sep 20, 2024

Uh oh!

hi-ogawa commented Sep 20, 2024

Uh oh!

vite-ecosystem-ci bot commented Sep 20, 2024

Uh oh!

sapphi-red left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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
	})
	}

Uh oh!

Comments

Conversation

hi-ogawa commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

bolt-new-by-stackblitz bot commented Sep 20, 2024

Uh oh!

hi-ogawa commented Sep 20, 2024

Uh oh!

AriPerkkio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hi-ogawa commented Sep 20, 2024

Uh oh!

hi-ogawa commented Sep 20, 2024

Uh oh!

vite-ecosystem-ci bot commented Sep 20, 2024

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hi-ogawa commented Sep 20, 2024 •

edited

Loading

AriPerkkio left a comment •

edited

Loading