fix: `__vite__mapDeps` code injection by hi-ogawa · Pull Request #15732 · vitejs/vite

hi-ogawa · 2024-01-28T04:56:01Z

Description

Closes __vite__mapDeps is not defined #15702

I copied and adjusted mapFileCommentRegex constant from https://github.com/thlorenz/convert-source-map/blob/1afbeee2f2a42a3747c31dfcfc355387afdf42e2/index.js#L14 so that it will only matches the last line of the source code. (EDIT: now prepending __vite__mapDeps at the top)

I manually verified that sourcemap is working via pnpm -C playground/js-sourcemap build + preview:

Show screenshot

2024-02-21.13-28-46.webm

Additional context

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines, especially the Pull Request Guidelines.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Update the corresponding documentation if needed.
Ideally, include relevant tests that fail without this PR but pass with it.

bolt-new-by-stackblitz · 2024-01-28T04:56:04Z

Run & review this pull request in StackBlitz Codeflow.

hi-ogawa

Related to the fact that mapFileCommentRegex matches anywhere in the file, I noticed that sourcemap: "inline" currently removes such string in the code as well:

vite/packages/vite/src/node/plugins/importAnalysisBuild.ts

Lines 556 to 561 in 013be31

    
           if (config.build.sourcemap === 'inline') { 
        
             chunk.code = chunk.code.replace( 
        
               convertSourceMap.mapFileCommentRegex, 
        
               '', 
        
             ) 
        
             chunk.code += `\n//# sourceMappingURL=${genSourceMapUrl(map)}`

For example:

// input
console.log(`
this is string literal
//# sourceMappingURL=1.css.map
`)

// output
console.log(`
this is string literal

`)

I suppose we can handle this issue separately?
(Probably the fix is as simple as hi-ogawa#1, but I didn't find a relevant test, so I'll try to come up with that separately)

packages/vite/src/node/plugins/importAnalysisBuild.ts

sapphi-red · 2024-02-20T10:34:17Z

packages/vite/src/node/plugins/importAnalysisBuild.ts


 const dynamicImportPrefixRE = /import\s*\(/

+// modified convert-source-map's `mapFileCommentRegex` to match only at the last line


mapFileCommentRegex to match only at the last line

This is correct for codes generated by Rollup itself. However, a plugin may inject a source map comment between some codes or that doesn't match this regex (e.g. /* sourceMappingURL=validURL *//* sourceMappingMeta={"foo":"bar"} */).
This is not allowed by the spec (although what "at the end of the source" means is not strictly defined). That said, it's supported by many runtimes (tc39/ecma426#64) and I guess it's better to avoid making this assumption.

In this case, how about prepending mapDepsCode instead of appending it? We need to deal with the Hashbang Grammar but that's not so hard.

In this case, how about prepending mapDepsCode instead of appending it? We need to deal with the Hashbang Grammar but that's not so hard.

Thanks for the review!
Right, I was also wondering about prepending it. I think I was worrying about hashbang as well, but probably prepending would be more straight forward overall. Let me try.

I updated a PR to prepend the code 7e714e1

The test case looks rather artificial but I (manually) confirmed that it covers a new code path and also sourcemap works for this case.

hi-ogawa · 2024-02-21T04:23:02Z

playground/js-sourcemap/vite.config.js

+        banner(chunk) {
+          if (chunk.name.endsWith('after-preload-dynamic-hashbang')) {
+            return '#!/usr/bin/env node'
+          }


I used banner to inject hashbang and test this case since having #! ... inside after-preload-dynamic-hashbang.js seems to cause a parse error somewhere in the pipeline. The error looks like this:

$ pnpm -C playground/js-sourcemap build > @vitejs/[email protected] build /home/hiroshi/code/others/vite/playground/js-sourcemap > vite build vite v5.1.3 building for production... ✓ 7 modules transformed. x Build failed in 56ms error during build: RollupError: Expected ident at error (file:///home/hiroshi/code/others/vite/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:337:30) at nodeConverters (file:///home/hiroshi/code/others/vite/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:2072:9) at convertNode (file:///home/hiroshi/code/others/vite/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:957:12) at convertProgram (file:///home/hiroshi/code/others/vite/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:948:48) at parseAstAsync (file:///home/hiroshi/code/others/vite/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/parseAst.js:2138:20) at Module.tryParseAsync (file:///home/hiroshi/code/others/vite/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:13357:21) at Module.setSource (file:///home/hiroshi/code/others/vite/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:12953:35) at ModuleLoader.addModuleSource (file:///home/hiroshi/code/others/vite/node_modules/.pnpm/[email protected]/node_modules/rollup/dist/es/shared/node-entry.js:17580:13) ELIFECYCLE Command failed with exit code 1.

hi-ogawa · 2024-02-21T04:25:53Z

packages/vite/src/node/plugins/importAnalysisBuild.ts


 const dynamicImportPrefixRE = /import\s*\(/

+// modified convert-source-map's `mapFileCommentRegex` to match only at the last line


I updated a PR to prepend the code 7e714e1

The test case looks rather artificial but I (manually) confirmed that it covers a new code path and also sourcemap works for this case.

sapphi-red

Thanks!

patak-dev · 2024-02-21T10:56:44Z

/ecosystem-ci run

vite-ecosystem-ci · 2024-02-21T11:10:57Z

📝 Ran ecosystem CI on 3e7e896: Open

suite	result	latest scheduled
analogjs	✅ success	✅ success
astro	✅ success	✅ success
histoire	✅ success	✅ success
ladle	✅ success	✅ success
laravel	✅ success	✅ success
marko	✅ success	✅ success
nuxt	✅ success	✅ success
nx	❌ failure	❌ failure
previewjs	✅ success	✅ success
qwik	✅ success	✅ success
rakkas	✅ success	✅ success
sveltekit	✅ success	✅ success
unocss	✅ success	✅ success
vike	✅ success	✅ success
vite-plugin-pwa	✅ success	✅ success
vite-plugin-react	✅ success	✅ success
vite-plugin-react-pages	✅ success	✅ success
vite-plugin-react-swc	✅ success	✅ success
vite-plugin-svelte	✅ success	✅ success
vite-plugin-vue	✅ success	✅ success
vite-setup-catalogue	✅ success	✅ success
vitepress	✅ success	✅ success
vitest	✅ success	✅ success

bluwy

LGTM! I also prefer the prepend approach

lisonge · 2024-03-24T07:04:23Z

packages/vite/src/node/plugins/importAnalysisBuild.ts

+            s.prependLeft(code.indexOf('\n') + 1, mapDepsCode)
          } else {
-            s.append(mapDepsCode)
+            s.prepend(mapDepsCode)


if fileDeps.length == 0, maybe we do not need exec s.prependLeft or prepend

because sometimes __vite__mapDeps is not used

I can't find how to remove the configuration of __vite__mapDeps

We can use [] instead of __vite__mapDeps([${renderedDeps.join(',')}]) if renderedDeps.length is 0. Then, we can skip injecting __vite__mapDeps. Contributions are welcome 👍

fix: fix __vite__mapDeps injected into a wrong position

6118a17

hi-ogawa changed the title ~~fix: fix __vite__mapDeps injected into a wrong position~~ fix: inject __vite__mapDeps before the correct source map comment Jan 28, 2024

hi-ogawa changed the title ~~fix: inject __vite__mapDeps before the correct source map comment~~ fix: inject __vite__mapDeps before the source map comment at the end Jan 28, 2024

hi-ogawa marked this pull request as ready for review January 28, 2024 05:05

test: tweak

013be31

hi-ogawa changed the title ~~fix: inject __vite__mapDeps before the source map comment at the end~~ fix: inject __vite__mapDeps before the source map comment at the last line Jan 28, 2024

hi-ogawa commented Jan 28, 2024

View reviewed changes

packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved

hi-ogawa added 2 commits January 28, 2024 15:05

chore: comment

32aad3d

refactor: minor

abaaefd

hi-ogawa mentioned this pull request Jan 28, 2024

fix: do not remove sourcemap comment used in string literal when sourcemap: 'inline' hi-ogawa/vite#1

Closed

sapphi-red reviewed Feb 20, 2024

View reviewed changes

hi-ogawa added 3 commits February 21, 2024 11:56

Merge branch 'main' into fix-mapDepsCode-append

6a1a4d2

chore: prepend __vite__mapDeps

7e714e1

test: snapshot

3e7e896

hi-ogawa changed the title ~~fix: inject __vite__mapDeps before the source map comment at the last line~~ fix: fix __vite__mapDeps code injection for sourcemap Feb 21, 2024

hi-ogawa changed the title ~~fix: fix __vite__mapDeps code injection for sourcemap~~ fix: fix __vite__mapDeps code injection Feb 21, 2024

hi-ogawa commented Feb 21, 2024

View reviewed changes

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 labels Feb 21, 2024

sapphi-red approved these changes Feb 21, 2024

View reviewed changes

sapphi-red requested review from bluwy and patak-dev February 21, 2024 10:43

patak-dev approved these changes Feb 21, 2024

View reviewed changes

patak-dev changed the title ~~fix: fix __vite__mapDeps code injection~~ fix: __vite__mapDeps code injection Feb 21, 2024

bluwy approved these changes Feb 21, 2024

View reviewed changes

bluwy merged commit aff54e1 into vitejs:main Feb 21, 2024

lisonge reviewed Mar 24, 2024

View reviewed changes

hi-ogawa mentioned this pull request Mar 26, 2024

fix: skip injecting __vite__mapDeps when it's not used #16271

Merged

9 tasks

	if (config.build.sourcemap === 'inline') {
	chunk.code = chunk.code.replace(
	convertSourceMap.mapFileCommentRegex,
	'',
	)
	chunk.code += `\n//# sourceMappingURL=${genSourceMapUrl(map)}`


		const dynamicImportPrefixRE = /import\s*\(/

		// modified convert-source-map's `mapFileCommentRegex` to match only at the last line

Uh oh!

Comments

Conversation

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

Uh oh!

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

bolt-new-by-stackblitz bot commented Jan 28, 2024

Uh oh!

hi-ogawa 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!

Uh oh!

sapphi-red Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

hi-ogawa Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

hi-ogawa Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

hi-ogawa Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

hi-ogawa Feb 21, 2024

Choose a reason for hiding this comment

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

patak-dev commented Feb 21, 2024

Uh oh!

vite-ecosystem-ci bot commented Feb 21, 2024

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

lisonge Mar 24, 2024 • 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!

sapphi-red Mar 24, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hi-ogawa commented Jan 28, 2024 •

edited

Loading

hi-ogawa left a comment •

edited

Loading

lisonge Mar 24, 2024 •

edited

Loading

sapphi-red Mar 24, 2024 •

edited

Loading