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

fix: inject __vite__mapDeps code before sourcemap file comment #15483

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Jan 2, 2024

Description

#14550 introduced __vite__mapDeps runtime function, which is appended to the chunk output during generateBundle hooks. Rollup's chunk already includes //# sourceMappingURL= comment at this point, thus MagicString.append would inject the code after such comment.

This PR fixes this issue by using MagicString.appendRight to inject right before the sourcemap comment if found.

Note that the issue manifested when sourcemap: true (i.e. separate sourcemap file) but not when sourcemap: "inline" since the comment was re-appended in inline case:

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

While testing on browsers by previewing playground/js-sourcemap locally, I found that //# sourceMappingURL comment appearing before __vite__mapDeps seems to be actually fine (at least for Chrome). So, I'm a little worried that the cause of the original issue might be something different. I'll ask for the further detail in the issue.

Here is what I tested (where both "before" and "after" look working however):

pnpm -C playground/js-sourcemap build
pnpm -C playground/js-sourcemap preview
Screenshots
  • before

image

image

  • after

image

image

Additional context

A few related PRs I looked up while attempting this fix:


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.

Copy link

stackblitz bot commented Jan 2, 2024

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

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for the fix.

While testing on browsers by previewing playground/js-sourcemap locally, I found that //# sourceMappingURL comment appearing before __vite__mapDeps seems to be actually fine (at least for Chrome). So, I'm a little worried that the cause of the original issue might be something different. I'll ask for the further detail in the issue.

I've seen other toolings before that mandated sourcemap comments to always come last, so I think it make sense to fix this still.

@sapphi-red
Copy link
Member

The spec says the comment should be at the end, so I think it's a valid fix, too.

The generated code may include a line at the end of the source
https://github.com/tc39/source-map-spec/blob/main/source-map-rev3.md#:~:text=The%20generated%20code%20may%20include%20a%20line%20at%20the%20end%20of%20the%20source%2C%20with%20the%20following%20form%3A

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: sourcemap Sourcemap support labels Jan 2, 2024
@sapphi-red sapphi-red merged commit d2aa096 into vitejs:main Jan 2, 2024
14 checks passed
@maximilianschmid
Copy link

@hi-ogawa works great, thanks!

@hi-ogawa hi-ogawa deleted the fix-inject-mapDeps-before-sourcemap-file-comment branch January 5, 2024 22:22
@hi-ogawa hi-ogawa mentioned this pull request Jan 24, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: sourcemap Sourcemap support 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.

Source maps broken due to injected __vite__mapDeps function after sourceMappingURL
4 participants