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: js fallback sourcemap content should be using original content #15135

Merged
merged 3 commits into from
Nov 24, 2023
Merged

fix: js fallback sourcemap content should be using original content #15135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8b6843
fix: js fallback sourcemap content should be using original content
sapphi-red Nov 24, 2023
220a267
chore: add comment
sapphi-red Nov 24, 2023
6cfe433
fix: check type === 'js'
sapphi-red Nov 24, 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/middlewares/transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
} from '../../utils'
import { send } from '../send'
import { ERR_LOAD_URL, transformRequest } from '../transformRequest'
import { applySourcemapIgnoreList } from '../sourcemap'
import { applySourcemapIgnoreList, getOriginalContent } from '../sourcemap'
import { isHTMLProxy } from '../../plugins/html'
import {
DEP_VERSION_RE,
Expand Down Expand Up @@ -205,12 +205,21 @@ export function transformMiddleware(
const type = isDirectCSSRequest(url) ? 'css' : 'js'
const isDep =
DEP_VERSION_RE.test(url) || depsOptimizer?.isOptimizedDepUrl(url)
let originalContent: string | undefined
if (type === 'js' && result.map == null) {
const filepath = (
await server.moduleGraph.getModuleByUrl(url, false)
)?.file
originalContent =
filepath != null ? await getOriginalContent(filepath) : undefined
}
return send(req, res, result.code, type, {
etag: result.etag,
// allow browser to cache npm deps!
cacheControl: isDep ? 'max-age=31536000,immutable' : 'no-cache',
headers: server.config.server.headers,
map: result.map,
originalContent,
})
}
}
Expand Down
24 changes: 14 additions & 10 deletions packages/vite/src/node/server/send.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export interface SendOptions {
cacheControl?: string
headers?: OutgoingHttpHeaders
map?: SourceMap | { mappings: '' } | null
/** only used when type === 'js' && map == null (when the fallback sourcemap is used) */
originalContent?: string
Copy link
Member

Choose a reason for hiding this comment

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

Should this be () => string in case it doesn't end up being used to save the file read?

Copy link
Member Author

@sapphi-red sapphi-red Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

send function is a sync function and is part of the public API. Therefore originalContent cannot be () => Promise<string>. To satisfy () => string, we need to change fsp.readFile to fs.readFileSync in getOriginalContent. But I guess async is better for perf.

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to find a way to avoid the perf regression. In a full .ts code base, there will now be a new read for each transformed file that won't be used, no? I think at one point we could have an internal send function that is async (everywhere internal usage could be changed to await), but given that it may be complex, for now we could add a guard at https://github.com/vitejs/vite/pull/15135/files#diff-6d94d6934079a4f09596acc9d3f3d38ea426c6f8e98cd766567335d42679ca7cR208, with a comment? Even if it is redundant, we at least save a few reads?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, OK. I noticed I was missing type === 'js'.

Copy link
Member Author

Choose a reason for hiding this comment

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

In a full .ts code base, there will now be a new read for each transformed file that won't be used, no?

Just to make it clear, this won't happen. .ts files will always have source maps, so map == null is always false.

}

export function send(
Expand Down Expand Up @@ -71,23 +73,25 @@ export function send(
}
// inject fallback sourcemap for js for improved debugging
// https://github.com/vitejs/vite/pull/13514#issuecomment-1592431496
else if (type === 'js' && (!map || map.mappings !== '')) {
// for { mappings: "" }, we don't inject fallback sourcemap
// because it indicates generating a sourcemap is meaningless
else if (type === 'js' && map == null) {
const code = content.toString()
// if the code has existing inline sourcemap, assume it's correct and skip
if (convertSourceMap.mapFileCommentRegex.test(code)) {
debug?.(`Skipped injecting fallback sourcemap for ${req.url}`)
} else {
const urlWithoutTimestamp = removeTimestampQuery(req.url!)
const ms = new MagicString(code)
content = getCodeWithSourcemap(
type,
code,
ms.generateMap({
source: path.basename(urlWithoutTimestamp),
hires: 'boundary',
includeContent: true,
}),
)
const map = ms.generateMap({
source: path.basename(urlWithoutTimestamp),
hires: 'boundary',
includeContent: !options.originalContent,
})
if (options.originalContent != null) {
map.sourcesContent = [options.originalContent]
}
content = getCodeWithSourcemap(type, code, map)
}
}

Expand Down
7 changes: 7 additions & 0 deletions packages/vite/src/node/server/sourcemap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ export async function injectSourcesContent(
}
}

export async function getOriginalContent(
filepath: string,
): Promise<string | undefined> {
if (virtualSourceRE.test(filepath)) return undefined
return await fsp.readFile(filepath, 'utf-8').catch(() => undefined)
}

export function genSourceMapUrl(map: SourceMap | string): string {
if (typeof map !== 'string') {
map = JSON.stringify(map)
Expand Down