Skip to content

Conversation

@isaacs
Copy link
Contributor

Fix: #48460

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

@anonriganonrig 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?

@isaacsisaacsforce-pushed the isaacs/expose-source-maps-line-lengths branch from b2376cf to 5873274CompareJune 15, 2023 04:04
@isaacs
Copy link
ContributorAuthor

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.

@isaacsisaacsforce-pushed the isaacs/expose-source-maps-line-lengths branch from 5873274 to 11b3b96CompareJune 15, 2023 16:50
@isaacsisaacsforce-pushed the isaacs/expose-source-maps-line-lengths branch from 11b3b96 to da4b63aCompareJune 24, 2023 19:33
@isaacsisaacsforce-pushed the isaacs/expose-source-maps-line-lengths branch from da4b63a to 1485dc8CompareJune 25, 2023 11:19
Copy link
Member

@legendecaslegendecas 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.

- v12.17.0
-->
#### `newSourceMap(payload)`
Copy link
Member

@legendecaslegendecasJul 5, 2023

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
ContributorAuthor

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

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!

@legendecaslegendecas added the source maps Issues and PRs related to source map support. label Jul 5, 2023
Document lineLengths argument Make lineLengths part of an option bag rather than a positional arg
Copy link
Member

@legendecaslegendecas left a comment

Choose a reason for hiding this comment

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

LGTM

@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 12, 2023
@github-actionsgithub-actionsbot 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
Copy link
Collaborator

@lpincalpinca 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-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 12, 2023
@nodejs-github-botnodejs-github-bot merged commit 9053943 into nodejs:mainJul 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 9053943

juanarbol pushed a commit that referenced this pull request Jul 13, 2023
Fix: #48460 PR-URL: #48461Fixes: #48460 Reviewed-By: Chengzhong Wu <[email protected]>
@juanarboljuanarbol mentioned this pull request Jul 13, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ruyadorno
Copy link
Member

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

@targostargos 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

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

nodejs-github-bot pushed a commit that referenced this pull request Feb 18, 2025
PR-URL: #57098Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit to aduh95/node that referenced this pull request Feb 18, 2025
PR-URL: nodejs#57098Fixes: nodejs#57094 Refs: nodejs#48461 Refs: nodejs#47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit to aduh95/node that referenced this pull request Feb 18, 2025
PR-URL: nodejs#57098Fixes: nodejs#57094 Refs: nodejs#48461 Refs: nodejs#47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit to aduh95/node that referenced this pull request Feb 18, 2025
PR-URL: nodejs#57098Fixes: nodejs#57094 Refs: nodejs#48461 Refs: nodejs#47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Feb 20, 2025
PR-URL: #57098 Backport-PR-URL: #57129Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
PR-URL: nodejs#57098Fixes: nodejs#57094 Refs: nodejs#48461 Refs: nodejs#47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Feb 24, 2025
PR-URL: #57098Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Feb 25, 2025
PR-URL: #57098Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 20, 2025
PR-URL: #57098 Backport-PR-URL: #57131Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit that referenced this pull request Apr 2, 2025
PR-URL: #57098Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit that referenced this pull request Apr 3, 2025
PR-URL: #57098Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
PR-URL: #57098Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
PR-URL: #57098Fixes: #57094 Refs: #48461 Refs: #47790 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

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