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.

Sign up for GitHub

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
Merged

fix(sourcemap): sourcemap is incorrect when sourcemap has sources: [null] #14588

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5089735
fix(node): sourcemap compatible rollup plugin
Dunqing Oct 12, 2023
77da6f5
fix(sourcemap): sourcemap is incorrect when sourcemap doesn’t include…
Dunqing Oct 12, 2023
a321ca2
test: update snap
Dunqing Oct 12, 2023
10e1d37
test: add plugin-sourcemap
Dunqing Oct 12, 2023
3bbb423
fix: test
Dunqing Oct 12, 2023
bd47303
Merge remote-tracking branch 'upstream/main' into fix/issue/13657
Dunqing Oct 15, 2023
bf10321
test: update test case
Dunqing Oct 15, 2023
7169664
Update playground/plugin-sourcemap/vite.config.js
Dunqing Nov 21, 2023
04c436e
Update packages/vite/src/node/server/pluginContainer.ts
Dunqing Nov 21, 2023
28ee56f
Merge branch 'main' into fix/issue/13657
Dunqing Nov 21, 2023
a856390
test: move test to js-sourcemap
Dunqing Nov 21, 2023
fca8146
fix: missing file
Dunqing Nov 21, 2023
80e2e4a
chore: fix lint
sapphi-red Nov 21, 2023
bf0b400
chore: format comment
sapphi-red Nov 21, 2023
e8a09a5
chore: remove console.log
sapphi-red Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/vite/src/node/server/pluginContainer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,16 @@ export async function createPluginContainer(
break
}
if (!combinedMap) {
combinedMap = m as SourceMap
const sm = m as SourceMap
if (sm.sources.length === 1 && !sm.sources[0]) {
Dunqing marked this conversation as resolved.
Show resolved Hide resolved
combinedMap = {
...sm,
sources: [this.filename],
sourcesContent: [this.originalCode],
}
} else {
combinedMap = sm
}
} else {
combinedMap = combineSourcemaps(cleanUrl(this.filename), [
m as RawSourceMap,
Comment on lines 604 to 605
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.

Expand Down
28 changes: 28 additions & 0 deletions playground/plugin-sourcemap/__tests__/plugin-sourcemap.spec.ts
View file
Open in desktop
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!

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { URL } from 'node:url'
import { expect, test } from 'vitest'
import {
extractSourcemap,
formatSourcemapForSnapshot,
isBuild,
page,
} from '~utils'

test.runIf(!isBuild)('correct sourcemap', async () => {
const res = await page.request.get(new URL('./foo.js', page.url()).href)
const js = await res.text()
expect(js).toContain('// add comment')
const map = extractSourcemap(js)
expect(formatSourcemapForSnapshot(map)).toMatchInlineSnapshot(`
{
"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;",
"sources": [
"foo.js",
],
"sourcesContent": [
"export const foo = 'foo'
",
],
"version": 3,
}
`)
})
1 change: 1 addition & 0 deletions playground/plugin-sourcemap/foo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const foo = 'foo'
5 changes: 5 additions & 0 deletions playground/plugin-sourcemap/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<div class="wrapper">
<h1>Plugin Sourcemap</h1>
</div>

<script type="module" src="./foo.js"></script>
15 changes: 15 additions & 0 deletions playground/plugin-sourcemap/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"name": "@vitejs/test-plugin-sourcemap",
"private": true,
"version": "0.0.0",
"type": "module",
"scripts": {
"dev": "vite",
"build": "vite build",
"debug": "node --inspect-brk ../../packages/vite/bin/vite",
"preview": "vite preview"
},
"devDependencies": {
"magic-string": "^0.30.3"
}
}
23 changes: 23 additions & 0 deletions playground/plugin-sourcemap/vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { defineConfig } from 'vite'
import MagicString from 'magic-string'

export default defineConfig({
plugins: [
{
name: 'sourcemap',
transform(code, id) {
if (id.includes('foo.js')) {
const ms = new MagicString(code)
ms.append('// add comment')
return {
code: ms.toString(),
map: ms.generateMap({ hires: true }),
Dunqing marked this conversation as resolved.
Show resolved Hide resolved
}
}
},
},
],
build: {
sourcemap: true,
},
})
6 changes: 6 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.