Skip to content

module: add SourceMap.lineLengths#48461

Merged
nodejs-github-bot merged 2 commits intonodejs:mainfrom
isaacs:isaacs/expose-source-maps-line-lengths
Jul 12, 2023
Merged

module: add SourceMap.lineLengths#48461
nodejs-github-bot merged 2 commits intonodejs:mainfrom
isaacs:isaacs/expose-source-maps-line-lengths

Conversation

@isaacs
Copy link
Contributor

@isaacs isaacs commented Jun 14, 2023

Fix: #48460

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 14, 2023
anonrig
anonrig reviewed Jun 15, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Can you update the documentation as well?

@isaacs isaacs force-pushed the isaacs/expose-source-maps-line-lengths branch from b2376cf to 5873274 Compare June 15, 2023 04:04
@isaacs
Copy link
Contributor Author

isaacs commented Jun 15, 2023

The failing tests in CI are a bit confusing, these all pass locally, and it looks like they're not running with source maps enabled for some reason. I'm not sure why anything I did there would have changed that though.

@isaacs isaacs force-pushed the isaacs/expose-source-maps-line-lengths branch from 5873274 to 11b3b96 Compare June 15, 2023 16:50
@isaacs isaacs force-pushed the isaacs/expose-source-maps-line-lengths branch from 11b3b96 to da4b63a Compare June 24, 2023 19:33
legendecas
legendecas reviewed Jun 25, 2023
View reviewed changes
@isaacs isaacs force-pushed the isaacs/expose-source-maps-line-lengths branch from da4b63a to 1485dc8 Compare June 25, 2023 11:19
legendecas
legendecas reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM % one nit.

doc/api/module.md Outdated
@@ -274,6 +274,7 @@ added:
#### `new SourceMap(payload)`
Copy link
Member

@legendecas legendecas Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The function signature should be updated to include the optional parameter too.

Suggested change
#### `new SourceMap(payload)`
#### `new SourceMap(payload[, lineLengths])`

As the generated code line lengths property is not part of the source maps spec, would it be more prudent to set the property with an option bag instead of a positional parameter?

Copy link
Contributor Author

@isaacs isaacs Jul 11, 2023

Choose a reason for hiding this comment

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

would it be more prudent to set the property with an option bag instead of a positional parameter?

No strong opinions here, but it's a pita to change this kind of thing later, so I'm feeling like it's probably a good idea, too. Anyone else wanna weight on it?

Copy link
Member

@legendecas legendecas Jul 12, 2023

Choose a reason for hiding this comment

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

@bcoe would you mind taking a look at this PR? Thank you!

@legendecas legendecas added the source maps Issues and PRs related to source map support. label Jul 5, 2023
@isaacs
fixup! module: add SourceMap.lineLengths
fed5a2f 
Document lineLengths argument
Make lineLengths part of an option bag rather than a positional arg
legendecas
legendecas approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 12, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 12, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52715/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 12, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52737/

@lpinca lpinca added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 12, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 12, 2023
@nodejs-github-bot nodejs-github-bot merged commit 9053943 into nodejs:main Jul 12, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 12, 2023

Landed in 9053943

juanarbol pushed a commit that referenced this pull request Jul 13, 2023
@isaacs @juanarbol
module: add SourceMap.lineLengths
6406f50 
Fix: #48460
PR-URL: #48461
Fixes: #48460
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
Merged
This was referenced Jul 16, 2023
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@isaacs @Ceres6
module: add SourceMap.lineLengths
2441f7a 
Fix: nodejs#48460
PR-URL: nodejs#48461
Fixes: nodejs#48460
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@isaacs @Ceres6
module: add SourceMap.lineLengths
7e908d3 
Fix: nodejs#48460
PR-URL: nodejs#48461
Fixes: nodejs#48460
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@ruyadorno
Copy link
Member

ruyadorno commented Sep 12, 2023

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 15, 2023
@targos
Copy link
Member

targos commented Nov 15, 2023

This is another PR that should have been labeled semver-minor.

@aduh95 aduh95 mentioned this pull request Feb 16, 2025
Merged
nodejs-github-bot pushed a commit that referenced this pull request Feb 18, 2025
@aduh95
doc: fix link and history of SourceMap sections
7a6b128 
PR-URL: #57098
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit to aduh95/node that referenced this pull request Feb 18, 2025
@aduh95
doc: fix link and history of SourceMap sections
ba6dfb4 
PR-URL: nodejs#57098
Fixes: nodejs#57094
Refs: nodejs#48461
Refs: nodejs#47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit to aduh95/node that referenced this pull request Feb 18, 2025
@aduh95
doc: fix link and history of SourceMap sections
402bf16 
PR-URL: nodejs#57098
Fixes: nodejs#57094
Refs: nodejs#48461
Refs: nodejs#47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit to aduh95/node that referenced this pull request Feb 18, 2025
@aduh95
doc: fix link and history of SourceMap sections
8f803af 
PR-URL: nodejs#57098
Fixes: nodejs#57094
Refs: nodejs#48461
Refs: nodejs#47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Feb 20, 2025
@aduh95 @marco-ippolito
doc: fix link and history of SourceMap sections
9e975f1 
PR-URL: #57098
Backport-PR-URL: #57129
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
@aduh95 @acidiney
doc: fix link and history of SourceMap sections
9e8af2b 
PR-URL: nodejs#57098
Fixes: nodejs#57094
Refs: nodejs#48461
Refs: nodejs#47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Feb 24, 2025
@aduh95 @targos
doc: fix link and history of SourceMap sections
6cf207e 
PR-URL: #57098
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Feb 25, 2025
@aduh95 @targos
doc: fix link and history of SourceMap sections
b696049 
PR-URL: #57098
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 20, 2025
@aduh95 @richardlau
doc: fix link and history of SourceMap sections
784da60 
PR-URL: #57098
Backport-PR-URL: #57131
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit that referenced this pull request Apr 2, 2025
@aduh95
doc: fix link and history of SourceMap sections
61bfe23 
PR-URL: #57098
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 added a commit that referenced this pull request Apr 3, 2025
@aduh95
doc: fix link and history of SourceMap sections
96c313e 
PR-URL: #57098
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@aduh95 @RafaelGSS
doc: fix link and history of SourceMap sections
851545b 
PR-URL: #57098
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@aduh95 @RafaelGSS
doc: fix link and history of SourceMap sections
6cc15b8 
PR-URL: #57098
Fixes: #57094
Refs: #48461
Refs: #47790
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig left review comments

@legendecas legendecas legendecas approved these changes

Assignees

No one assigned

Labels

commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. source maps Issues and PRs related to source map support.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Expose lineLengths from source map cache

7 participants

@isaacs @nodejs-github-bot @ruyadorno @targos @anonrig @legendecas @lpinca