Skip to content

Revert "Fix sourcemap lookup when line numbers are enabled (#81)"#106

Merged
nsavoire merged 1 commit intomainfrom
nsavoire/fix_webpack_sourcemaps
Jun 14, 2023
Merged

Revert "Fix sourcemap lookup when line numbers are enabled (#81)"#106
nsavoire merged 1 commit intomainfrom
nsavoire/fix_webpack_sourcemaps

Conversation

@nsavoire
Copy link
Copy Markdown

This reverts commit cfebbb3.
This commit resulted in failure to get original function name back for source maps generated by webpack.

@nsavoire nsavoire requested a review from r1viollet June 14, 2023 16:48
@nsavoire nsavoire requested review from Qard and szegedi as code owners June 14, 2023 16:48
@github-actions
Copy link
Copy Markdown

Overall package size

Self size: 3.04 MB
Deduped: 3.92 MB
No deduping: 3.92 MB

Dependency sizes

name version self size total size
pprof-format 2.0.7 588.12 kB 588.12 kB
source-map 0.7.4 226 kB 226 kB
split 1.0.1 12.29 kB 24.82 kB
p-limit 3.1.0 7.75 kB 13.78 kB
delay 5.0.0 11.17 kB 11.17 kB
pify 5.0.0 8.87 kB 8.87 kB
node-gyp-build 3.9.0 8.81 kB 8.81 kB

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@nsavoire nsavoire added bug Something isn't working semver-minor Usually minor non-breaking improvements labels Jun 14, 2023
@pr-commenter
Copy link
Copy Markdown

pr-commenter bot commented Jun 14, 2023

Benchmarks

Benchmark execution time: 2023-06-14 16:51:11

Comparing candidate commit b978f09 in PR branch nsavoire/fix_webpack_sourcemaps with baseline commit 60585ba in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 114 metrics, 66 unstable metrics.

Copy link
Copy Markdown

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM

@nsavoire nsavoire merged commit 10eb724 into main Jun 14, 2023
@nsavoire nsavoire deleted the nsavoire/fix_webpack_sourcemaps branch June 14, 2023 18:55
nsavoire added a commit that referenced this pull request Jun 15, 2023
nsavoire added a commit that referenced this pull request Jun 15, 2023
szegedi added a commit that referenced this pull request Mar 13, 2026
Add tests for SourceMapper.mappingInfo with a synthetic webpack-style
single-line bundle to document the known limitation introduced by #248.

Background
----------
PR #81 changed originalPositionFor to always try LEAST_UPPER_BOUND
first (then fall back to GREATEST_LOWER_BOUND). This was reverted in
#106 because it broke webpack source maps: for real non-zero columns
LEAST_UPPER_BOUND finds the *next* mapping (≥ column) rather than the
one at the column, returning wrong function names.

PR #248 fixed that regression by using LEAST_UPPER_BOUND only when
column === 0, and GREATEST_LOWER_BOUND otherwise. This correctly
handles Node.js ≥ 25 where V8's LineTick struct carries real column
numbers.

Residual limitation (Node.js < 25)
-----------------------------------
On Node.js < 25, the LineTick struct has no column field. The C++ layer
therefore always emits column=0 for every LineTick sample. With
column=0, the sourcemapper uses LEAST_UPPER_BOUND, which finds the
*first* mapping on the line. In a webpack bundle (all output on one
line) every function maps to the same first source function in the map.

This is not a regression vs. the pre-#248 state: before #248, those
functions were simply unmapped (column=0 + GREATEST_LOWER_BOUND →
nothing ≤ 0 → null → falls back to generated name/file). Both outcomes
are imperfect; #248 trades "unmapped" for "mapped to first function",
which may or may not be preferable depending on the use case.

The two new tests pin both behaviours explicitly so any future change
to this logic is immediately visible.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working semver-minor Usually minor non-breaking improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants