Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module: add SourceMap.lineLengths #48461

Merged
merged 2 commits into from
Jul 12, 2023
Merged

module: add SourceMap.lineLengths #48461

merged 2 commits into from
Jul 12, 2023

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

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!

@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

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

@nodejs-github-bot
Copy link
Collaborator

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

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

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

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 12, 2023
@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.

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
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. 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