Repository navigation
feat: emit the source map scopes field - #275
Conversation
🦋 Changeset detectedLatest commit: 306c28f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
==========================================
+ Coverage 98.91% 99.01% +0.09%
==========================================
Files 26 27 +1
Lines 5708 6266 +558
Branches 741 791 +50
==========================================
+ Hits 5646 6204 +558
Misses 60 60
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will regress 16 benchmarks
|
A bundler rewrites an imported binding into a namespace read, and a
source map only maps positions, so a debugger cannot resolve the name
the source used. The Scopes proposal answers that by naming, per scope,
the expression each variable evaluates to.
Add map({ scopes: true }). A source declares what its names read through
a new OriginalSource argument, the chunk stream carries it beside the
source content, and the field is built while the mappings are written
rather than decoded back out of them.
Requiring it from the two map assembly helpers put the encoder and its lookup table in the module graph of every caller, which the memory benchmarks measured as a constant cost on sources that never emit the field.
The streaming writer reaches none of it, so a map handed in from elsewhere went untested: the early returns, a segment naming no source or one the map lacks, a source resumed after another, a name the map already carries, and a value too large for one base64 digit.
The cases required the helper module directly, so the getter that exposes it on `util` was never run.
Also choose the mapping callback once instead of testing for the scopes
writer per segment. Interleaved against main on one machine, that test
cost OriginalSource.map({ columns: true }) a reproducible 1-3%; with the
callback chosen up front the same measurement is neutral.
`tsc` emits `@private` as a TypeScript `private` member, which makes classes from two installed copies incompatible; `main` dropped these tags when it moved to generated types (#291). Regenerate `types/` for the scopes changes. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F
`main` runs the tests with `node:test` (#292); convert `expect` to `assert` so the file runs under `node --test` and the Jest adapter. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F
941c833 to
a894b28
Compare
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
WalkthroughThe change adds optional scope bindings to Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Scope maps can be invalid when generated output lacks a trailing newline, or report incorrect expressions when composed sources share a name. Fix these opt-in map-generation cases before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Scope metadata can be lost when a cached source is composed again, or associated with the wrong code when distinct sources share a name. No security exploit or new external exposure was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 37868bec-e87d-41be-8bb7-63c915bcbc7e
📒 Files selected for processing (21)
.changeset/source-map-scopes.mdREADME.mdlib/CachedSource.jslib/ConcatSource.jslib/OriginalSource.jslib/ReplaceSource.jslib/Source.jslib/helpers/getFromStreamChunks.jslib/helpers/scopes.jslib/helpers/streamAndGetSourceAndMap.jslib/helpers/streamChunks.jslib/helpers/streamChunksOfCombinedSourceMap.jslib/index.jstest/scopes.jstypes.d.tstypes/OriginalSource.d.tstypes/Source.d.tstypes/helpers/scopes.d.tstypes/helpers/streamAndGetSourceAndMap.d.tstypes/helpers/streamChunks.d.tstypes/index.d.ts
💤 Files with no reviewable changes (1)
- types/helpers/streamAndGetSourceAndMap.d.ts
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.
The scopes writer closed the open range at column 0 of the last line. When the output does not end with a newline, a range that starts on that line began after that point, so its end encoded a negative column delta, which the unsigned VLQ writer turned into 4294967292 and an invalid `scopes` field. Pass the generated end column through `finish`. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F
|
CodSpeed Performance Analysis is red, but not because of this PR. The first report compared against
The one re-run also mixed environments. It shows 16 regressions and 24 improvements, a different set each time. To check without the runner differences, I measured
Retained memory is identical for all of these. Nothing reproduces the 3× memory growth or the 10–18% slowdowns CodSpeed reports, and several of those are in constructors this PR doesn't change. The regressions can be acknowledged on CodSpeed. Generated by Claude Code |
Summary
A bundler rewrites an imported binding into a namespace read, and a source map only maps positions, so a debugger cannot resolve the name the source actually used. This adds
map({ scopes: true }), emitting the ECMA-426 Scopes field: a source declares what its names read through a new thirdOriginalSourceargument, the chunk stream carries that beside the source content, and the field is built while the mappings are written rather than decoded back out of them. Needed by webpack/webpack#21999, which otherwise has to carry its own VLQ codec and re-parse every asset'smappings.It also fixes a pre-existing cache bug it would otherwise trip over:
CachedSource's fast-path key inspected onlycolumns, so any two option sets differing elsewhere collided — withscopesthat meant a map built without the field could be served to a caller asking for it, and vice versa.What kind of change does this PR introduce?
feat.
Did you add tests for your changes?
Yes —
test/scopes.js: the option is opt-in, bindings surviveConcatSource,ReplaceSourceand a primedCachedSource, neither cache order leaks or drops the field, and the streaming encoder is asserted byte-identical to decoding a finished map.Does this PR introduce a breaking change?
No.
OnSourcegains an optional fourth argument andMapOptionsan optionalscopesflag; without it nothing is collected or emitted, and the existing 89,895 tests and 1,373 snapshots pass unchanged.If relevant, what needs to be documented once your changes are merged or what have you already documented in this PR?
map({ scopes: true }), theOriginalSourcethird constructor argument, and theutil.scopeshelpers.Use of AI
Yes. Claude Code was used to move the encoder out of webpack, wire the option through the streaming path, and write the tests. The design — a plain boolean option with the bindings riding on the sources, rather than a callback that a cache key cannot see — was mine, and the
CachedSourcebugs above were found by testing that decision rather than assumed.🤖 Generated with Claude Code
https://claude.ai/code/session_01C5WxsJzknrdkVzHV3rL1Uv
Generated by Claude Code
Summary by CodeRabbit
scopesoption.OriginalSourceaccepts optional scope bindings, which are preserved through composed and replaced sources.scopesoption, scope bindings, and when scope data appears in generated maps.