src,lib: retrieve parsed source map url from v8 by legendecas · Pull Request #44798 · nodejs/node · GitHub

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.

Review requested:

@nodejs/modules

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.

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

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

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

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');

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

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

LGTM

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

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

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

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

Landed in 6bdc101. Thank you for the reviews!

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 <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Jan Krems <jan.krems@gmail.com>

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 force-pushed the parse-sourcemap branch from a4ab0b5 to 936cc1e Compare September 26, 2022 16:58

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 force-pushed the parse-sourcemap branch from 936cc1e to df4e7df Compare September 26, 2022 17:02

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 reviewed Sep 26, 2022

View reviewed changes

Comment thread lib/internal/vm.js Outdated

Comment thread lib/internal/vm.js Outdated

joyeecheung reviewed Sep 28, 2022

View reviewed changes

fixup! src,lib: retrieve parsed source map url from v8

86de757

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2022

hybrist reviewed Sep 29, 2022

View reviewed changes

github-actions Bot mentioned this pull request Sep 30, 2022

CI Reliability 2022-09-30 nodejs/reliability#390

Open

17 tasks

joyeecheung approved these changes Sep 30, 2022

View reviewed changes

This was referenced Oct 1, 2022

CI Reliability 2022-10-01 nodejs/reliability#391

Open

CI Reliability 2022-10-02 nodejs/reliability#392

Open

jasnell reviewed Oct 2, 2022

View reviewed changes

Comment thread doc/api/vm.md Outdated

jasnell approved these changes Oct 2, 2022

View reviewed changes

github-actions Bot mentioned this pull request Oct 3, 2022

CI Reliability 2022-10-03 nodejs/reliability#393

Open

9 tasks

fixup! src,lib: retrieve parsed source map url from v8

3566c6f

legendecas force-pushed the parse-sourcemap branch from 9c6839a to 3566c6f Compare October 5, 2022 07:48

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2022

anonrig reviewed Oct 5, 2022

View reviewed changes

Comment thread lib/internal/vm.js

anonrig approved these changes Oct 22, 2022

View reviewed changes

hybrist approved these changes Oct 23, 2022

View reviewed changes

legendecas closed this Oct 23, 2022

legendecas deleted the parse-sourcemap branch October 23, 2022 14:37

RafaelGSS mentioned this pull request Nov 1, 2022

v19.1.0 proposal #45269

Merged

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

+                if (result
+                        ->Set(parsing_context,
+                              env->source_map_url_string(),
+                              fn->GetScriptOrigin().SourceMapUrl())

-                // This is needed so that we don't match sourceMappingURL in string literals.
-                while ((match = RegExpPrototypeExec(kSourceMappingURLMagicComment, content))) {
-                  lastMatch = match;
+                if (sourceMapURL === undefined) {

-                // This is needed so that we don't match sourceMappingURL in string literals.
-                while ((match = RegExpPrototypeExec(kSourceMappingURLMagicComment, content))) {
-                  lastMatch = match;
+                if (sourceMapURL === undefined) {

src,lib: retrieve parsed source map url from v8 by legendecas · Pull Request #44798 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src,lib: retrieve parsed source map url from v8#44798legendecas wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom legendecas:parse-sourcemaplegendecas/node:parse-sourcemapCopy head branch name to clipboard

Conversation

legendecas commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 26, 2022

Uh oh!

Uh oh!

Uh oh!

joyeecheung Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

legendecas Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 29, 2022

Uh oh!

hybrist Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

joyeecheung Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legendecas Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 5, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 6, 2022

Uh oh!

legendecas commented Oct 11, 2022

Uh oh!

legendecas commented Oct 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

src,lib: retrieve parsed source map url from v8#44798
legendecas wants to merge 3 commits into
nodejs:mainfrom
legendecas:parse-sourcemap

legendecas commented Sep 26, 2022 •
edited
Loading

joyeecheung Sep 30, 2022 •
edited
Loading

legendecas Oct 5, 2022 •
edited
Loading

joyeecheung Sep 30, 2022 •
edited
Loading