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

src,lib: retrieve parsed source map url from v8 #44798

Closed
wants to merge 3 commits into from
Closed

src,lib: retrieve parsed source map url from v8 #44798

wants to merge 3 commits into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Sep 26, 2022 •
edited
Loading

V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.

Retrieving source map URL of a Module needs a V8 patch, working on
https://chromium-review.googlesource.com/c/v8/v8/+/3917379.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 26, 2022
@legendecas
src,lib: retrieve parsed source map url from v8
df4e7df 
V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.
@legendecas legendecas added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. source maps Issues and PRs related to source map support. labels Sep 26, 2022
ljharb
ljharb reviewed Sep 26, 2022
View reviewed changes
lib/internal/vm.js Outdated Show resolved Hide resolved
lib/internal/vm.js Outdated Show resolved Hide resolved
joyeecheung
joyeecheung reviewed Sep 28, 2022
View reviewed changes
src/node_contextify.cc
if (result
->Set(parsing_context,
env->source_map_url_string(),
fn->GetScriptOrigin().SourceMapUrl())
Copy link
Member

Choose a reason for hiding this comment

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

Does this work with the debug build? I think this being empty could trigger a DCHECK.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test on no magic comment presents. Verified with debug build that no DCHECK are violated.

doc/api/vm.md Show resolved Hide resolved
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2022
@nodejs-github-bot
Copy link
Collaborator

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

jkrems
jkrems reviewed Sep 29, 2022
View reviewed changes
lib/internal/source_map/source_map_cache.js
// This is needed so that we don't match sourceMappingURL in string literals.
while ((match = RegExpPrototypeExec(kSourceMappingURLMagicComment, content))) {
lastMatch = match;
if (sourceMapURL === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it by design that we still do the regex parse when V8 told us that there is no source map? If so, should we add a test case, e.g.:

checkSourceMapUrl(`
function myFunc() {}
`
//# sourceMappingURL=sourcemap.json
`;
`, 'sourcemap.json');

Copy link
Member

@joyeecheung joyeecheung Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this fallback is kept here to handle the ES modules?

Copy link
Member Author

@legendecas legendecas Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jkrems @joyeecheung thanks for the suggestion. We don't need to apply the regex again when the parse result of the script indicates that no source mapping URL is available. I've updated the code to eliminate the duplicated scans.

It is true that the fallback is left here to handle the ES modules.

@github-actions github-actions bot mentioned this pull request Sep 30, 2022
Open
17 tasks
joyeecheung
joyeecheung approved these changes Sep 30, 2022
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM

lib/internal/source_map/source_map_cache.js
// This is needed so that we don't match sourceMappingURL in string literals.
while ((match = RegExpPrototypeExec(kSourceMappingURLMagicComment, content))) {
lastMatch = match;
if (sourceMapURL === undefined) {
Copy link
Member

@joyeecheung joyeecheung Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this fallback is kept here to handle the ES modules?

This was referenced Oct 1, 2022
Open
Open
jasnell
jasnell reviewed Oct 2, 2022
View reviewed changes
doc/api/vm.md Outdated Show resolved Hide resolved
@github-actions github-actions bot mentioned this pull request Oct 3, 2022
Open
9 tasks
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@legendecas
Copy link
Member Author

@jkrems @joyeecheung would you mind taking a look again at this with the updates on #44798 (comment)? thank you!

@legendecas
Copy link
Member Author

Landed in 6bdc101. Thank you for the reviews!

legendecas added a commit that referenced this pull request Oct 23, 2022
@legendecas
src,lib: retrieve parsed source map url from v8
6bdc101 
V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.

PR-URL: #44798
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@legendecas legendecas closed this Oct 23, 2022
@legendecas legendecas deleted the parse-sourcemap branch October 23, 2022 14:37
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@legendecas @RafaelGSS
src,lib: retrieve parsed source map url from v8
30ca63b 
V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.

PR-URL: #44798
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@legendecas @RafaelGSS
src,lib: retrieve parsed source map url from v8
7a31ae8 
V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.

PR-URL: #44798
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@legendecas @danielleadams
src,lib: retrieve parsed source map url from v8
9d6e7ec 
V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.

PR-URL: #44798
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@legendecas @danielleadams
src,lib: retrieve parsed source map url from v8
744edd7 
V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.

PR-URL: #44798
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@legendecas @danielleadams
src,lib: retrieve parsed source map url from v8
12779b3 
V8 already parses the source map magic comments. Currently, only scripts
and functions expose the parsed source map URLs. It is unnecessary to
parse the source map magic comments again when the parsed information is
available.

PR-URL: #44798
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@anonrig anonrig anonrig approved these changes

@jkrems jkrems jkrems approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. source maps Issues and PRs related to source map support. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@legendecas @nodejs-github-bot @ljharb @jasnell @jkrems @anonrig @joyeecheung