Enhance source map handling in NLS plugin and related components#298894
Enhance source map handling in NLS plugin and related components#298894
Conversation
- Introduced adjustments for source maps in the NLS plugin to ensure accurate mapping after placeholder replacements. - Implemented deferred processing for source maps to handle edits more effectively, preserving unmapped segments. - Updated tests to validate column mappings and ensure correctness in both minified and non-minified builds. - Improved documentation to reflect changes in source map generation and adjustments.
There was a problem hiding this comment.
Pull request overview
This PR updates the build/next esbuild-based bundling pipeline to improve source map correctness across NLS placeholder transformations and post-processing edits, and adds test coverage for column/line mapping drift (including minified output).
Changes:
- NLS pipeline now tracks placeholder replacement edits and applies
adjustSourceMapso maps stay accurate afterpostProcessNLS. adjustSourceMapis reworked to handle multi-line edits and to preserve unmapped segments +sourceRoot.- Bundling defers
.maphandling until after.jsprocessing to ensure edits are available; tests/docs updated accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| build/next/working.md | Updates documentation describing the source map strategy and known remaining issues. |
| build/next/test/private-to-property.test.ts | Adds coverage for multi-line edit behavior affecting subsequent generated line mappings. |
| build/next/test/nls-sourcemap.test.ts | Adds end-to-end tests for post-processed NLS column drift and minified mapping. |
| build/next/private-to-property.ts | Reworks adjustSourceMap to adjust via offsets (supports newline-spanning edits) and preserves unmapped mappings + sourceRoot. |
| build/next/nls-plugin.ts | Makes postProcessNLS return edits, expands inline sourcemap generation to per-column identity mappings, and refactors placeholder replacement to track edits. |
| build/next/index.ts | Defers .map writes and applies chained adjustSourceMap passes for mangle + NLS edits. |
| build/gulpfile.vscode.ts | Adds CI-driven toggles for CDN sourcemap base URL usage and stripping packaged .map files. |
| // Collect all matches first, then apply from back to front so that byte | ||
| // offsets remain valid. Each match becomes a TextEdit in terms of the | ||
| // ORIGINAL content offsets, which is what adjustSourceMap expects. | ||
|
|
There was a problem hiding this comment.
The comment says edits are applied “from back to front”, but the implementation builds parts and applies replacements in increasing offset order. The forward assembly is fine, but the comment should be updated to avoid misleading future changes (especially since offsets are intentionally kept in original-content coordinates).
| // Source maps don't interpolate columns — each query resolves to the | ||
| // last segment with generatedColumn <= queryColumn. A single mapping | ||
| // at edit-end would cause every subsequent column on this line to | ||
| // collapse to that one original position. Add per-column identity | ||
| // mappings from edit-end to the next edit (or end of line) so that | ||
| // esbuild's source-map composition preserves fine-grained accuracy. | ||
| const nextBound = i + 1 < lineEdits.length ? lineEdits[i + 1].startCol : lines[line].length; | ||
| for (let origCol = edit.endCol; origCol < nextBound; origCol++) { | ||
| generator.addMapping({ | ||
| generated: { line: smLine, column: origCol + cumulativeShift }, | ||
| original: { line: smLine, column: origCol }, | ||
| source: filePath, | ||
| }); | ||
| } |
There was a problem hiding this comment.
generateNLSSourceMap now adds an identity mapping for every column after each edit. This is O(lineLength) in mappings per edited line and can blow up inline source map size and composition cost (esbuild has to compose these during bundling). Consider using sparser “checkpoint” mappings (e.g. every N columns) or another strategy that avoids per-column segments while still preventing the post-edit collapse behavior.
| const sourceFilterPattern = stripSourceMapsInPackagingTasks | ||
| ? ['**', '!**/*.{js,css}.map'] | ||
| : ['**']; | ||
| const sources = es.merge(src, extensions) | ||
| .pipe(filter(['**', '!**/*.{js,css}.map'], { dot: true })); | ||
| .pipe(filter(sourceFilterPattern, { dot: true })); |
There was a problem hiding this comment.
In CI, stripSourceMapsInPackagingTasks removes *.map files for all packaging tasks, including non-minified ones. However runEsbuildBundle only rewrites sourceMappingURL to the CDN when minified && useCdnSourceMapsForPackagingTasks, so non-minified bundles will still reference local .map files that are no longer packaged. Consider only stripping maps when sourceMapBaseUrl is set (or also rewrite/strip the sourceMappingURL comments for non-minified builds).
| // by converting each mapping's old generated (line, col) to a byte offset, | ||
| // adjusting the offset for the edits, then converting back to (line, col) in | ||
| // the post-edit coordinate system. | ||
|
|
||
| const oldLineStarts = buildLineStarts(originalCode); | ||
| const newLineStarts = buildLineStartsAfterEdits(originalCode, edits); | ||
|
|
||
| // Precompute cumulative byte-shift after each edit for binary search |
There was a problem hiding this comment.
Several comments refer to edit positions as “byte offsets”, but the start/end indices and string.length in JS/TS are UTF-16 code-unit offsets. Calling these “byte” offsets is misleading (and would be wrong for non-ASCII content). Consider renaming the terminology in the comment to “string offsets” or “UTF-16 offsets” to match the actual units used throughout adjustSourceMap.
| // by converting each mapping's old generated (line, col) to a byte offset, | |
| // adjusting the offset for the edits, then converting back to (line, col) in | |
| // the post-edit coordinate system. | |
| const oldLineStarts = buildLineStarts(originalCode); | |
| const newLineStarts = buildLineStartsAfterEdits(originalCode, edits); | |
| // Precompute cumulative byte-shift after each edit for binary search | |
| // by converting each mapping's old generated (line, col) to a string offset | |
| // (in UTF-16 code units), adjusting the offset for the edits, then converting | |
| // back to (line, col) in the post-edit coordinate system. | |
| const oldLineStarts = buildLineStarts(originalCode); | |
| const newLineStarts = buildLineStartsAfterEdits(originalCode, edits); | |
| // Precompute cumulative offset shift (in UTF-16 code units) after each edit for binary search |
fixes #297252