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(sourcemap): sourcemap is incorrect when sourcemap has sources: [null] #14588

Merged
merged 15 commits into from
Jan 8, 2024

Conversation

Dunqing
Copy link
Contributor

@Dunqing Dunqing commented Oct 12, 2023

Description

Fixes: #13657

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.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • 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).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Dunqing Dunqing marked this pull request as ready for review October 12, 2023 08:31
@Dunqing Dunqing changed the title fix(node): sourcemap compatible rollup plugin fix(sourcemap): sourcemap is incorrect when sourcemap doesn’t include souces Oct 12, 2023
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.

Thank you for the PR!

playground/plugin-sourcemap/vite.config.js Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Would you merge this playground to js-sourcemap playground to reduce the number of playgrounds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Comment on lines 592 to 593
combinedMap = combineSourcemaps(cleanUrl(this.filename), [
m as RawSourceMap,
Copy link
Member

Choose a reason for hiding this comment

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

Does combineSourcemaps work without doing the same normalization (i.e. setting [this.filename]) above? This happens when a module is transformed by a plugin then transformed by another plugin that uses MagicString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sources are absolutely present when entering here Because the first source map has added sources or the sources themselves exist

Copy link
Member

Choose a reason for hiding this comment

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

combineSourcemaps has two inputs. The second one (combinedMap) wouldn't have sources: ['']. But the first one (m) might have sources: [''], no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the second one definitely have

Copy link
Member

Choose a reason for hiding this comment

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

I've actually tested it now by changing the code like below and renaming zoo.js to zoo.ts.

          console.log('Before', m, combinedMap)
          combinedMap = combineSourcemaps(cleanUrl(this.filename), [
            m as RawSourceMap,
            combinedMap as RawSourceMap,
          ]) as SourceMap
          console.log('After', combinedMap)

I got the following result:

Before SourceMap {
  version: 3,
  file: undefined,
  sources: [ '' ],
  sourcesContent: undefined,
  names: [],
  mappings: 'AAAA,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC,CAAC;'
} {
  version: 3,
  sources: [ 'D:/documents/GitHub/vite/playground/js-sourcemap/zoo.ts' ],
  sourcesContent: [ "export const zoo: string = 'zoo'\n" ],
  mappings: 'AAAO,aAAM,MAAc;',
  names: []
}
After SourceMap {
  version: 3,
  mappings: 'AAAO,aAAM,MAAc',
  names: [],
  sourceRoot: undefined,
  sources: [ 'D:/documents/GitHub/vite/playground/js-sourcemap/zoo.ts' ],
  sourcesContent: [ "export const zoo: string = 'zoo'\n" ],
  file: 'D:/documents/GitHub/vite/playground/js-sourcemap/zoo.ts'
}

It seems it works at least for this case.

@sapphi-red sapphi-red added rollup plugin compat p3-minor-bug An edge case that only affects very specific usage (priority) feat: sourcemap Sourcemap support labels Nov 20, 2023
@sapphi-red sapphi-red changed the title fix(sourcemap): sourcemap is incorrect when sourcemap doesn’t include souces fix(sourcemap): sourcemap is incorrect when sourcemap has sources: [null] Nov 20, 2023
@sapphi-red sapphi-red requested a review from bluwy January 2, 2024 08:05
@patak-dev patak-dev merged commit f8c6a34 into vitejs:main Jan 8, 2024
10 checks passed
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) rollup plugin compat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The @rollup/plugin-inject plugin causes null print path for the console
4 participants