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: breakpoints in JS not working #13514

Merged
merged 6 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion packages/vite/rollup.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ function createCjsConfig(isProduction: boolean) {
...Object.keys(pkg.dependencies),
...(isProduction ? [] : Object.keys(pkg.devDependencies)),
],
plugins: [...createNodePlugins(false, false, false), bundleSizeLimit(123)],
plugins: [...createNodePlugins(false, false, false), bundleSizeLimit(155)],
})
}

Expand Down
4 changes: 2 additions & 2 deletions packages/vite/src/node/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ export interface ViteDevServer {
*/
ssrTransform(
code: string,
inMap: SourceMap | null,
inMap: SourceMap | { mappings: '' } | null,
url: string,
originalCode?: string,
): Promise<TransformResult | null>
Expand Down Expand Up @@ -385,7 +385,7 @@ export async function _createServer(
resolvedUrls: null, // will be set on listen
ssrTransform(
code: string,
inMap: SourceMap | null,
inMap: SourceMap | { mappings: '' } | null,
url: string,
originalCode = code,
) {
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/server/middlewares/indexHtml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ const devHtmlHook: IndexHtmlTransformHook = async (
const result = await server!.pluginContainer.transform(code, mod.id!)
let content = ''
if (result) {
if (result.map) {
if (result.map && 'version' in result.map) {
if (result.map.mappings) {
await injectSourcesContent(
result.map,
Expand Down
45 changes: 29 additions & 16 deletions packages/vite/src/node/server/pluginContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export interface PluginContainer {
inMap?: SourceDescription['map']
ssr?: boolean
},
): Promise<{ code: string; map: SourceMap | null }>
): Promise<{ code: string; map: SourceMap | { mappings: '' } | null }>
load(
id: string,
options?: {
Expand Down Expand Up @@ -481,7 +481,7 @@ export async function createPluginContainer(
typeof err.loc?.column === 'number'
) {
const rawSourceMap = ctx._getCombinedSourcemap()
if (rawSourceMap) {
if (rawSourceMap && 'version' in rawSourceMap) {
const traced = new TraceMap(rawSourceMap as any)
const { source, line, column } = originalPositionFor(traced, {
line: Number(err.loc.line),
Expand Down Expand Up @@ -525,7 +525,7 @@ export async function createPluginContainer(
originalCode: string
originalSourcemap: SourceMap | null = null
sourcemapChain: NonNullable<SourceDescription['map']>[] = []
combinedMap: SourceMap | null = null
combinedMap: SourceMap | { mappings: '' } | null = null

constructor(filename: string, code: string, inMap?: SourceMap | string) {
super()
Expand All @@ -540,7 +540,7 @@ export async function createPluginContainer(
}
}

_getCombinedSourcemap(createIfNull = false) {
_getCombinedSourcemap() {
if (
debugSourcemapCombine &&
debugSourcemapCombineFilter &&
Expand All @@ -553,12 +553,26 @@ export async function createPluginContainer(
}

let combinedMap = this.combinedMap
// { mappings: '' }
if (
combinedMap &&
!('version' in combinedMap) &&
combinedMap.mappings === ''
) {
this.sourcemapChain.length = 0
return combinedMap
}

for (let m of this.sourcemapChain) {
if (typeof m === 'string') m = JSON.parse(m)
if (!('version' in (m as SourceMap))) {
// { mappings: '' }
if ((m as SourceMap).mappings === '') {
combinedMap = { mappings: '' }
break
}
// empty, nullified source map
combinedMap = this.combinedMap = null
this.sourcemapChain.length = 0
combinedMap = null
break
Comment on lines +569 to 576
Copy link
Member Author

Choose a reason for hiding this comment

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

This part was not correct. { mappings: '' } is different from null. So it shouldn't be changed to null.
For example, if this.sourcemapChain is [{ mappings: '' }, validSourcemap], the combined result should be { mappings: '' } instead of validSourcemap.

}
if (!combinedMap) {
Expand All @@ -570,15 +584,6 @@ export async function createPluginContainer(
]) as SourceMap
}
}
if (!combinedMap) {
return createIfNull
? new MagicString(this.originalCode).generateMap({
includeContent: true,
hires: 'boundary',
source: cleanUrl(this.filename),
})
: null
}
if (combinedMap !== this.combinedMap) {
this.combinedMap = combinedMap
this.sourcemapChain.length = 0
Expand All @@ -587,7 +592,15 @@ export async function createPluginContainer(
}

getCombinedSourcemap() {
return this._getCombinedSourcemap(true) as SourceMap
const map = this._getCombinedSourcemap()
if (!map || (!('version' in map) && map.mappings === '')) {
return new MagicString(this.originalCode).generateMap({
includeContent: true,
hires: 'boundary',
source: cleanUrl(this.filename),
})
}
return map
}
}

Expand Down
16 changes: 14 additions & 2 deletions packages/vite/src/node/server/send.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import type {
} from 'node:http'
import getEtag from 'etag'
import type { SourceMap } from 'rollup'
import MagicString from 'magic-string'
import { removeTimestampQuery } from '../utils'
import { getCodeWithSourcemap } from './sourcemap'

const alias: Record<string, string | undefined> = {
Expand All @@ -18,7 +20,7 @@ export interface SendOptions {
etag?: string
cacheControl?: string
headers?: OutgoingHttpHeaders
map?: SourceMap | null
map?: SourceMap | { mappings: '' } | null
}

export function send(
Expand Down Expand Up @@ -56,10 +58,20 @@ export function send(
}

// inject source map reference
if (map && map.mappings) {
if (map && 'version' in map && map.mappings) {
if (type === 'js' || type === 'css') {
content = getCodeWithSourcemap(type, content.toString(), map)
}
} else {
if (type === 'js' && (!map || map.mappings !== '')) {
const urlWithoutTimestamp = removeTimestampQuery(req.url!)
const ms = new MagicString(content.toString())
content = getCodeWithSourcemap(
type,
content.toString(),
ms.generateMap({ source: urlWithoutTimestamp, hires: 'boundary' }),
)
}
Comment on lines +66 to +74
Copy link
Member Author

Choose a reason for hiding this comment

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

This part is the main part of the change. Other changes are to pass { mappings: '' } as-is to this part.

}

res.statusCode = 200
Expand Down
32 changes: 20 additions & 12 deletions packages/vite/src/node/server/transformRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const debugCache = createDebugger('vite:cache')

export interface TransformResult {
code: string
map: SourceMap | null
map: SourceMap | { mappings: '' } | null
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is a breaking change.
But this type was not correctly declared and actually transformRequest could return { mappings: '' } if a load hook returned a map with that and no transform hook transformed that request.

etag?: string
deps?: string[]
dynamicDeps?: string[]
Expand Down Expand Up @@ -286,15 +286,23 @@ async function loadAndTransform(
map = transformResult.map
}

if (map && mod.file) {
map = (typeof map === 'string' ? JSON.parse(map) : map) as SourceMap
if (map.mappings) {
await injectSourcesContent(map, mod.file, logger)
let normalizedMap: SourceMap | { mappings: '' } | null
if (typeof map === 'string') {
normalizedMap = JSON.parse(map)
} else if (map) {
normalizedMap = map as SourceMap | { mappings: '' }
} else {
normalizedMap = null
}

if (normalizedMap && 'version' in normalizedMap && mod.file) {
if (normalizedMap.mappings) {
await injectSourcesContent(normalizedMap, mod.file, logger)
}

const sourcemapPath = `${mod.file}.map`
applySourcemapIgnoreList(
map,
normalizedMap,
sourcemapPath,
config.server.sourcemapIgnoreList,
logger,
Expand All @@ -303,16 +311,16 @@ async function loadAndTransform(
if (path.isAbsolute(mod.file)) {
for (
let sourcesIndex = 0;
sourcesIndex < map.sources.length;
sourcesIndex < normalizedMap.sources.length;
++sourcesIndex
) {
const sourcePath = map.sources[sourcesIndex]
const sourcePath = normalizedMap.sources[sourcesIndex]
if (sourcePath) {
// Rewrite sources to relative paths to give debuggers the chance
// to resolve and display them in a meaningful way (rather than
// with absolute paths).
if (path.isAbsolute(sourcePath)) {
map.sources[sourcesIndex] = path.relative(
normalizedMap.sources[sourcesIndex] = path.relative(
path.dirname(mod.file),
sourcePath,
)
Expand All @@ -326,12 +334,12 @@ async function loadAndTransform(

const result =
ssr && !server.config.experimental.skipSsrTransform
? await server.ssrTransform(code, map as SourceMap, url, originalCode)
? await server.ssrTransform(code, normalizedMap, url, originalCode)
: ({
code,
map,
map: normalizedMap,
etag: getEtag(code, { weak: true }),
} as TransformResult)
} satisfies TransformResult)

// Only cache the result if the module wasn't invalidated while it was
// being processed, so it is re-processed next time if it is stale
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/ssr/ssrModuleLoader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ async function instantiateModule(
}

let sourceMapSuffix = ''
if (result.map) {
if (result.map && 'version' in result.map) {
const moduleSourceMap = Object.assign({}, result.map, {
// currently we need to offset the line
// https://github.com/nodejs/node/issues/43047#issuecomment-1180632750
Expand Down
13 changes: 9 additions & 4 deletions packages/vite/src/node/ssr/ssrTransform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const hashbangRE = /^#!.*\n/

export async function ssrTransform(
code: string,
inMap: SourceMap | null,
inMap: SourceMap | { mappings: '' } | null,
url: string,
originalCode: string,
options?: TransformOptions,
Expand All @@ -51,7 +51,7 @@ export async function ssrTransform(

async function ssrTransformJSON(
code: string,
inMap: SourceMap | null,
inMap: SourceMap | { mappings: '' } | null,
): Promise<TransformResult> {
return {
code: code.replace('export default', `${ssrModuleExportsKey}.default =`),
Expand All @@ -63,7 +63,7 @@ async function ssrTransformJSON(

async function ssrTransformScript(
code: string,
inMap: SourceMap | null,
inMap: SourceMap | { mappings: '' } | null,
url: string,
originalCode: string,
): Promise<TransformResult | null> {
Expand Down Expand Up @@ -275,7 +275,12 @@ async function ssrTransformScript(
})

let map = s.generateMap({ hires: 'boundary' })
if (inMap && inMap.mappings && inMap.sources.length > 0) {
if (
inMap &&
inMap.mappings &&
'sources' in inMap &&
inMap.sources.length > 0
) {
map = combineSourcemaps(url, [
{
...map,
Expand Down
3 changes: 1 addition & 2 deletions playground/css-sourcemap/__tests__/css-sourcemap.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@ describe.runIf(isServe)('serve', () => {
new URL('./linked-with-import.css', page.url()).href,
)
const content = await res.text()
const lines = content.trim().split('\n')
expect(lines[lines.length - 1]).not.toMatch(/^\/\/#/)
expect(content).not.toMatch('//#s*sourceMappingURL')
},
)

Expand Down
12 changes: 10 additions & 2 deletions playground/js-sourcemap/__tests__/js-sourcemap.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,16 @@ if (!isBuild) {
test('js', async () => {
const res = await page.request.get(new URL('./foo.js', page.url()).href)
const js = await res.text()
const lines = js.split('\n')
expect(lines[lines.length - 1].includes('//')).toBe(false) // expect no sourcemap
const map = extractSourcemap(js)
expect(formatSourcemapForSnapshot(map)).toMatchInlineSnapshot(`
{
"mappings": "AAAA,MAAM,CAAC,KAAK,CAAC,GAAG,CAAC,CAAC,CAAC,CAAC,GAAG,CAAC;",
"sources": [
"/foo.js",
],
"version": 3,
}
`)
})

test('ts', async () => {
Expand Down