fix(compile-sfc): handle inline template source map in prod build #12701
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/compiler-sfc
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
@vue/compat
vue
commit: |
edison1105
added
the
🔨 p3-minor-bug
WalkthroughThe changes introduce enhanced source map merging in the Vue SFC compiler to accurately map both Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant compileScript
participant compileTemplate
participant mergeSourceMaps
User->>compileScript: Compile SFC with inline template
compileScript->>compileTemplate: Compile template, get code and source map
compileScript->>compileScript: Generate script setup source map
compileScript->>mergeSourceMaps: Merge script and template maps with offset
mergeSourceMaps-->>compileScript: Return merged source map
compileScript-->>User: Return compiled content and merged source map
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/compiler-sfc/__tests__/compileScript.spec.ts (1)
701-719: Importrefto keep the fixture realistic and future-proofThe new test relies on
ref()but omitsimport { ref } from 'vue'While the compiler currently tolerates the missing import, adding it prevents false positives if future binding-analysis becomes stricter and makes the snippet closer to real-world usage.
<script setup> - const count = ref(0) + import { ref } from 'vue' + const count = ref(0) </script>packages/compiler-sfc/src/compileScript.ts (1)
1303-1340: Avoid touching private fields ofsource-map-js& release consumers
_sources,_sourceRoot,_fileare private internals; relying on them risks breakage on minor upgrades.
The public API already covers these needs:- const generator = new SourceMapGenerator() + const generator = new SourceMapGenerator({ + file: scriptMap.file, + sourceRoot: scriptMap.sourceRoot, + }) … - ;(generator as any)._sourceRoot = scriptMap.sourceRoot - ;(generator as any)._file = scriptMap.file + // no direct property mutation necessaryAlso, each
SourceMapConsumershould be.destroy()-ed (async) or.close()(sync) when done to free WASM/worker resources:const consumer = await new SourceMapConsumer(map) try { … } finally { consumer.destroy?.() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
packages/compiler-sfc/__tests__/compileScript.spec.ts(2 hunks)packages/compiler-sfc/__tests__/compileTemplate.spec.ts(1 hunks)packages/compiler-sfc/__tests__/utils.ts(1 hunks)packages/compiler-sfc/src/compileScript.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / unit-test-windows
- GitHub Check: test / e2e-test
🔇 Additional comments (1)
packages/compiler-sfc/__tests__/compileTemplate.spec.ts (1)
9-9: Centralize position utility via shared import
Great refactor—removing the localgetPositionInCodedefinition and importing it from./utilsDRYs up the code and ensures consistency across tests. The relative path is accurate given the__tests__directory structure.
| export function getPositionInCode( | ||
| code: string, | ||
| token: string, | ||
| expectName: string | boolean = false, | ||
| ): Pos { | ||
| const generatedOffset = code.indexOf(token) | ||
| let line = 1 | ||
| let lastNewLinePos = -1 | ||
| for (let i = 0; i < generatedOffset; i++) { | ||
| if (code.charCodeAt(i) === 10 /* newline char code */) { | ||
| line++ | ||
| lastNewLinePos = i | ||
| } | ||
| } | ||
| const res: Pos = { | ||
| line, | ||
| column: | ||
| lastNewLinePos === -1 | ||
| ? generatedOffset | ||
| : generatedOffset - lastNewLinePos - 1, | ||
| } | ||
| if (expectName) { | ||
| res.name = typeof expectName === 'string' ? expectName : token | ||
| } | ||
| return res | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against “token not found” and CRLF line endings
indexOf() can return -1 ‑ leading to line=1 and column=-1, which later feeds SourceMapConsumer.originalPositionFor() with negative values.
In addition, the loop only treats \n (LF) as a newline; on Windows a CRLF sequence will mis-count lines.
+ if (generatedOffset === -1) {
+ throw new Error(`Token "${token}" not found in supplied code snippet`)
+ }
for (let i = 0; i < generatedOffset; i++) {
- if (code.charCodeAt(i) === 10 /* newline char code */) {
+ const ch = code.charCodeAt(i)
+ // 10 → \n | 13 → \r
+ if (ch === 10 || ch === 13) {
line++
lastNewLinePos = i
}
}This eliminates silent mis-mappings and makes the helper reliable across OSes.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/compiler-sfc/__tests__/utils.ts between lines 50 and 75, the
function getPositionInCode does not handle the case when the token is not found,
causing indexOf to return -1 and resulting in invalid line and column values.
Additionally, it only counts '\n' as a newline, which fails on Windows CRLF line
endings. Fix this by adding a guard to return a default or error position if
token is not found, and update the loop to correctly count both '\r\n' and '\n'
as newlines to ensure accurate line and column calculation across OSes.
| }) as unknown as RawSourceMap) | ||
| : undefined | ||
| // merge source maps of the script setup and template in inline mode | ||
| if (templateMap && map) { | ||
| const offset = content.indexOf(returned) | ||
| const templateLineOffset = | ||
| content.slice(0, offset).split(/\r?\n/).length - 1 | ||
| map = mergeSourceMaps(map, templateMap, templateLineOffset) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential mismatch when returned appears earlier in content
content.indexOf(returned) assumes the first occurrence marks the template insertion point.
A comment, string literal or earlier return block containing the same substring would shift templateLineOffset, producing wrong mappings.
Safer approach: remember the exact start line when you append returned, e.g., obtain ctx.s.lineToOffset() before appendRight, or return the offset from compileTemplate and pass it through.
🤖 Prompt for AI Agents
In packages/compiler-sfc/src/compileScript.ts around lines 1030 to 1038, the
code uses content.indexOf(returned) to find the template insertion point, which
can be incorrect if the returned string appears earlier in content. To fix this,
track the exact start line or offset at the moment you append the returned
template string, for example by calling ctx.s.lineToOffset() before appendRight
or by modifying compileTemplate to return the offset, then use this precise
offset to calculate templateLineOffset for merging source maps accurately.
close #12682
close vitejs/vite-plugin-vue#500
Summary by CodeRabbit
New Features
Tests
Refactor
Documentation