fix: map functions without closing brace mapping#120
Conversation
📝 WalkthroughWalkthroughAdds a pnpm patch for istanbul-lib-source-maps that implements a multi-line fallback for missing end-position mappings; mirrors that fallback in the repository's location mapping logic and adds tests plus updated coverage snapshots. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Locator as Locator (src/location.ts)
participant Lib as istanbul-lib-source-maps (patched)
participant Map as SourceMap
Note over Test,Locator: Request mapping for a start/end range
Test->>Locator: getLoc(start, end)
Locator->>Lib: originalPositionTryBoth(start)
Lib-->>Locator: mappedStart
Locator->>Lib: originalPositionTryBoth(end)
alt end mapping found
Lib-->>Locator: mappedEnd
else end mapping missing
Locator->>Lib: loop request for lines (line-1 ... startLine) with large column
Lib-->>Locator: first non-null mappedEnd or null
end
Locator->>Map: convert original positions back to generated (bump end)
Locator-->>Test: mapped location (or null)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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: 0
🧹 Nitpick comments (1)
src/location.ts (1)
95-101: Fix typo in comment.Line 96: "doesnt" should be "doesn't".
Otherwise, the multi-line fallback logic looks correct. It properly handles the case where TypeScript and other transpilers omit closing braces from source maps by searching previous lines for valid mappings.
🔎 Proposed fix
- // e.g. tsc that doesnt include } in source maps + // e.g. tsc that doesn't include } in source maps
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.jsonpatches/istanbul-lib-source-maps.patchsrc/location.tstest/location.test.tstest/svelte.test.tstest/vue.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/location.test.ts (1)
src/location.ts (1)
Locator(27-167)
🔇 Additional comments (5)
test/svelte.test.ts (1)
22-24: LGTM! Snapshot update reflects improved function mapping.The increased function and statement counts (from 3 to 4 functions, 5 to 6 statements) align with the PR's objective to correctly map functions without closing brace mappings.
test/vue.test.ts (1)
22-24: LGTM! Snapshot correctly reflects the mapping improvements.The metrics now detect 1 function (previously 0/0) and corresponding increases in lines and statements, consistent with the improved end-position mapping behavior.
package.json (1)
81-83: LGTM! Patch configuration is correct.The
patchedDependenciesentry properly references the patch file for istanbul-lib-source-maps, enabling the multi-line fallback behavior in the external dependency.test/location.test.ts (1)
106-140: LGTM! Test effectively validates the fallback behavior.The test correctly verifies that
getLoccan map positions when end mappings are absent, exercising the multi-line fallback logic introduced in this PR.patches/istanbul-lib-source-maps.patch (1)
8-36: LGTM! Patch correctly implements the fallback logic.The changes mirror the implementation in
src/location.ts:
- Variable changed to
letto enable reassignment (line 13)- Multi-line backward search added (lines 20-30)
- Proper null check after exhausting search (lines 31-33)
Note: Patches to dependencies may require updates when bumping the istanbul-lib-source-maps version. Consider monitoring the upstream PR (istanbuljs/istanbuljs#837) for merge status.
| }, | ||
| "patchedDependencies": { | ||
| "istanbul-lib-source-maps": "patches/istanbul-lib-source-maps.patch" |
There was a problem hiding this comment.
I added this patch temporarily to make the test pass.
| "functions": "0/0 (100%)", | ||
| "lines": "0/2 (0%)", | ||
| "statements": "0/2 (0%)", | ||
| "functions": "0/1 (0%)", |
There was a problem hiding this comment.
v-for is transformed to _renderList(..., () => { ... }) and this callback is now mapped correctly.
| { | ||
| "branches": "0/0 (100%)", | ||
| "functions": "0/3 (0%)", | ||
| "functions": "0/4 (0%)", |
There was a problem hiding this comment.
Similar to vue, #each is transformed to $.each(..., ..., () => ..., () => ..., () => { ... }) and this last callback is now mapped correctly.
There was a problem hiding this comment.
This and the Vue test are here mostly to verify this library and Istanbul packages remap the same code similarly. In Svelte and Vue, I wouldn't necessarily call any of this "correct", but it's what their compilers output:
commit: |
| if (end === null) { | ||
| // search the previous lines as the mapping was not found on the same line | ||
| // e.g. tsc that doesnt include } in source maps | ||
| for (let line = endNeedle.line; line > 0 && end === null; line--) { |
There was a problem hiding this comment.
Would replacing line > 0 with line >= startNeedle.line make more sense? I guess it should always match once line === startNeedle.line is reached though. We should not look for end mappings above start line.
There was a problem hiding this comment.
We can replace this with line >= startNeedle.line. I used line > 0 as startNeedle wasn't available in istanbul-lib-source-maps.
| // search the previous lines as the mapping was not found on the same line | ||
| // e.g. tsc that doesnt include } in source maps | ||
| for (let line = endNeedle.line; line > 0 && end === null; line--) { | ||
| end = getPosition({ line, column: Number.MAX_SAFE_INTEGER }, this.#map); |
There was a problem hiding this comment.
Does Number.MAX_SAFE_INTEGER have some special meaning here or could this be Infinity too? It's just to search the end of the line?
There was a problem hiding this comment.
Number.MAX_SAFE_INTEGER was the one that came up in my head first. I'll replace it with Infinity. It would have a small overhead because of float & integer mix, but I guess it would be negligible.
There was a problem hiding this comment.
Looks good, thanks!
I have a feeling this might cause issues in some cases where nodes are nested, and we are looking for the outer one only. It might not come up with functions, but this remapping logic is used for all cases. But I can't really point out any examples to demonstrate this. Let's merge this as-is and see if it breaks anything.
| { | ||
| "branches": "0/0 (100%)", | ||
| "functions": "0/3 (0%)", | ||
| "functions": "0/4 (0%)", |
There was a problem hiding this comment.
This and the Vue test are here mostly to verify this library and Istanbul packages remap the same code similarly. In Svelte and Vue, I wouldn't necessarily call any of this "correct", but it's what their compilers output:
2 participants
Same as istanbuljs/istanbuljs#837
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.