lib: don't match `sourceMappingURL` in strings #44658

alan-agius4 · 2022-09-15T13:18:21Z

Prior to this change sourceMappingURL in string where being matched by the RegExp which caused sourcemaps not be loaded when using the --enable-source-maps flag. This commit changes the RegExp to match the last occurrence.

Fixes: #44654

lib/internal/source_map/source_map_cache.js

legendecas

I found that v8 has been more strict than what Node.js does to parsing the source map magic comments. AFAICT, we don't have a written spec on the magic comment formats. I'll bring this topic to the TC39 tooling discussion to see what can we do to reduce the divergence here.

Anyway, this change looks good to me! Thank you for working on this.

nodejs-github-bot · 2022-09-15T17:32:30Z

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

legendecas · 2022-09-15T17:41:39Z

test/fixtures/source-map/typescript-sourcemapping_url_string.js

+"use strict";
+const content = '/*# sourceMappingURL=';
+throw new Error('an exception.');
+//# sourceMappingURL=typescript-sourcemapping_url_string.js.map


With a deeper dig, I found that this works because of the trailing mark $ in the RegExp: i.e. the search begins from the tail of the source content.

If a statement of a string literal that contains a magic comment is present after the actual source map magic comment, this still matches the string literal instead of the actual magic comment.

But I assume we should always take the last magic comment present in the source content, and the magic comment always comes after the real source content. Maybe we can simply add the $ mark in the RegExp instead.

(sorry for the churn, it's late night and I really need to go to sleep 😴)

Thanks for the feedback. I did go down the root at the beginning but found out that when both sourceMappingURL and sourceURL are specified.
Ex:

node/test/fixtures/source-map/tabs.js

Lines 55 to 56 in 85b46e1

//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoidGFicy5qcyIsInNvdXJjZVJvb3QiOiIiLCJzb3VyY2VzIjpbInRhYnMuY29mZmVlIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiI7QUFBYTtFQUFBO0FBQUEsTUFBQSxLQUFBLEVBQUEsSUFBQSxFQUFBLElBQUEsRUFBQSxHQUFBLEVBQUEsTUFBQSxFQUFBLFFBQUEsRUFBQSxJQUFBLEVBQUE7O0VBQ2IsTUFBQSxHQUFXOztFQUNYLFFBQUEsR0FBVzs7RUFHWCxJQUFnQixRQUFoQjs7SUFBQSxNQUFBLEdBQVMsQ0FBQyxHQUFWO0dBTGE7OztFQVFiLE1BQUEsR0FBUyxRQUFBLENBQUMsQ0FBRCxDQUFBO1dBQU8sQ0FBQSxHQUFJO0VBQVgsRUFSSTs7O0VBV2IsSUFBQSxHQUFPLENBQUMsQ0FBRCxFQUFJLENBQUosRUFBTyxDQUFQLEVBQVUsQ0FBVixFQUFhLENBQWIsRUFYTTs7O0VBY2IsSUFBQSxHQUNDO0lBQUEsSUFBQSxFQUFRLElBQUksQ0FBQyxJQUFiO0lBQ0EsTUFBQSxFQUFRLE1BRFI7SUFFQSxJQUFBLEVBQVEsUUFBQSxDQUFDLENBQUQsQ0FBQTthQUFPLENBQUEsR0FBSSxNQUFBLENBQU8sQ0FBUDtJQUFYO0VBRlIsRUFmWTs7O0VBb0JiLElBQUEsR0FBTyxRQUFBLENBQUMsTUFBRCxFQUFBLEdBQVMsT0FBVCxDQUFBO1dBQ04sS0FBQSxDQUFNLE1BQU4sRUFBYyxPQUFkO0VBRE0sRUFwQk07OztFQXdCYixJQUFHLElBQUg7SUFDQyxLQUFBLENBQU0sWUFBTixFQUREO0dBeEJhOzs7RUE0QmIsS0FBQTs7QUFBUztJQUFBLEtBQUEsc0NBQUE7O21CQUFBLElBQUksQ0FBQyxJQUFMLENBQVUsR0FBVjtJQUFBLENBQUE7OztBQTVCSSIsInNvdXJjZXNDb250ZW50IjpbIiMgQXNzaWdubWVudDpcbm51bWJlciAgID0gNDJcbm9wcG9zaXRlID0gdHJ1ZVxuXG4jIENvbmRpdGlvbnM6XG5udW1iZXIgPSAtNDIgaWYgb3Bwb3NpdGVcblxuIyBGdW5jdGlvbnM6XG5zcXVhcmUgPSAoeCkgLT4geCAqIHhcblxuIyBBcnJheXM6XG5saXN0ID0gWzEsIDIsIDMsIDQsIDVdXG5cbiMgT2JqZWN0czpcbm1hdGggPVxuXHRyb290OiAgIE1hdGguc3FydFxuXHRzcXVhcmU6IHNxdWFyZVxuXHRjdWJlOiAgICh4KSAtPiB4ICogc3F1YXJlIHhcblxuIyBTcGxhdHM6XG5yYWNlID0gKHdpbm5lciwgcnVubmVycy4uLikgLT5cblx0cHJpbnQgd2lubmVyLCBydW5uZXJzXG5cbiMgRXhpc3RlbmNlOlxuaWYgdHJ1ZVxuXHRhbGVydCBcIkkga25ldyBpdCFcIlxuXG4jIEFycmF5IGNvbXByZWhlbnNpb25zOlxuY3ViZXMgPSAobWF0aC5jdWJlIG51bSBmb3IgbnVtIGluIGxpc3QpXG4iXX0=

//# sourceURL=/Users/bencoe/oss/coffee-script-test/tabs.coffee

I did however, go back and amend the RegExp to use the global flag which makes this simpler.

aduh95 · 2022-09-16T08:45:57Z

@alan-agius4 don't hesitate not to force push when pushing more updates, it creates a poor reviewer experience because GitHub is not able to show the diff since the last review, it's a bit frustrating to have to "start over" the review again and again. If instead, you push additional commits to the branch, that would be better for me (and all commits will be squashed into one upon landing anyway).

Prior to this change `sourceMappingURL` in string where being matched by the RegExp which caused sourcemaps not be loaded when using the `--enable-source-maps` flag. This commit changes the RegExp to match the last occurrence. Fixes: nodejs#44654

alan-agius4 · 2022-09-16T09:05:31Z

@alan-agius4 don't hesitate not to force push when pushing more updates, it creates a poor reviewer experience because GitHub is not able to show the diff since the last review, it's a bit frustrating to have to "start over" the review again and again. If instead, you push additional commits to the branch, that would be better for me (and all commits will be squashed into one upon landing anyway).

Oops sorry about that. I will keep this in mind for the next time.

aduh95

This deserves some comments as I don't think it would be obvious to future reader why we use a loop.

alan-agius4 · 2022-09-16T09:36:51Z

This deserves some comments as I don't think it would be obvious to future reader why we use a loop.

Added comments.

nodejs-github-bot · 2022-09-18T10:53:47Z

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

test/message/source_map_sourcemapping_url_string.out

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

nodejs-github-bot · 2022-09-18T16:58:13Z

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

nodejs-github-bot · 2022-09-19T01:54:12Z

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

alan-agius4 · 2022-09-20T19:53:28Z

Anything needed from my end to make this green? As the failures seems unrelated to this change.

Should I rebase?

nodejs-github-bot · 2022-09-20T21:17:14Z

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

aduh95 · 2022-09-20T21:17:18Z

Should I rebase?

Please don't, it wouldn't help in this case. CI failures seem indeed to be unrelated, I've resumed the CI, hopefully that'd be enough to turn it green.

Prior to this change `sourceMappingURL` in string where being matched by the RegExp which caused sourcemaps not be loaded when using the `--enable-source-maps` flag. This commit changes the RegExp to match the last occurrence. Fixes: #44654 PR-URL: #44658 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

legendecas · 2022-09-25T16:11:46Z

Landed in e6018e2. Thank you for your contribution!

Prior to this change `sourceMappingURL` in string where being matched by the RegExp which caused sourcemaps not be loaded when using the `--enable-source-maps` flag. This commit changes the RegExp to match the last occurrence. Fixes: #44654 PR-URL: #44658 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

juanarbol · 2022-10-04T00:22:17Z

This depends on #43875; marking this as "backport-blocked-v16.x"

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 15, 2022

alan-agius4 force-pushed the fix/js-source-maps branch 2 times, most recently from 09d29a2 to 51f24d5 Compare September 15, 2022 13:22

aduh95 reviewed Sep 15, 2022

View reviewed changes

lib/internal/source_map/source_map_cache.js Outdated Show resolved Hide resolved

alan-agius4 force-pushed the fix/js-source-maps branch from 51f24d5 to 1c54fff Compare September 15, 2022 13:29

aduh95 reviewed Sep 15, 2022

View reviewed changes

lib/internal/source_map/source_map_cache.js Outdated Show resolved Hide resolved

alan-agius4 force-pushed the fix/js-source-maps branch from 1c54fff to 3e43e1c Compare September 15, 2022 13:47

cola119 added the source maps Issues and PRs related to source map support. label Sep 15, 2022

alan-agius4 force-pushed the fix/js-source-maps branch from 3e43e1c to d308119 Compare September 15, 2022 16:55

aduh95 reviewed Sep 15, 2022

View reviewed changes

lib/internal/source_map/source_map_cache.js Outdated Show resolved Hide resolved

alan-agius4 force-pushed the fix/js-source-maps branch from d308119 to 15db6d8 Compare September 15, 2022 17:03

legendecas approved these changes Sep 15, 2022

View reviewed changes

aduh95 requested a review from bcoe September 15, 2022 17:25

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 15, 2022

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

legendecas reviewed Sep 15, 2022

View reviewed changes

alan-agius4 force-pushed the fix/js-source-maps branch from 15db6d8 to 098f579 Compare September 16, 2022 08:30

alan-agius4 force-pushed the fix/js-source-maps branch from 098f579 to 680947c Compare September 16, 2022 09:03

aduh95 reviewed Sep 16, 2022

View reviewed changes

fixup! lib: don't match sourceMappingURL in strings

3b104a4

aduh95 approved these changes Sep 18, 2022

View reviewed changes

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

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

legendecas approved these changes Sep 18, 2022

View reviewed changes

legendecas reviewed Sep 18, 2022

View reviewed changes

test/message/source_map_sourcemapping_url_string.out Outdated Show resolved Hide resolved

fixup! lib: don't match sourceMappingURL in strings

3bf0e8d

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

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

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

legendecas closed this Sep 25, 2022

alan-agius4 deleted the fix/js-source-maps branch September 25, 2022 18:43

RafaelGSS mentioned this pull request Sep 26, 2022

v18.10.0 proposal #44799

Merged

juanarbol added the backport-blocked-v16.x label Oct 4, 2022

BenceSzalai mentioned this pull request Sep 2, 2023

External source map is not supported privatenumber/tsx#340

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: don't match `sourceMappingURL` in strings #44658

lib: don't match `sourceMappingURL` in strings #44658

alan-agius4 commented Sep 15, 2022

legendecas left a comment •

edited

nodejs-github-bot commented Sep 15, 2022

legendecas Sep 15, 2022 •

edited

alan-agius4 Sep 16, 2022

aduh95 commented Sep 16, 2022

alan-agius4 commented Sep 16, 2022

aduh95 left a comment

alan-agius4 commented Sep 16, 2022

nodejs-github-bot commented Sep 18, 2022

nodejs-github-bot commented Sep 18, 2022

nodejs-github-bot commented Sep 19, 2022

alan-agius4 commented Sep 20, 2022

nodejs-github-bot commented Sep 20, 2022

aduh95 commented Sep 20, 2022 •

edited

legendecas commented Sep 25, 2022 •

edited

juanarbol commented Oct 4, 2022

lib: don't match sourceMappingURL in strings #44658

lib: don't match sourceMappingURL in strings #44658

Conversation

alan-agius4 commented Sep 15, 2022

legendecas left a comment • edited

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 15, 2022

legendecas Sep 15, 2022 • edited

Choose a reason for hiding this comment

alan-agius4 Sep 16, 2022

Choose a reason for hiding this comment

aduh95 commented Sep 16, 2022

alan-agius4 commented Sep 16, 2022

aduh95 left a comment

Choose a reason for hiding this comment

alan-agius4 commented Sep 16, 2022

nodejs-github-bot commented Sep 18, 2022

nodejs-github-bot commented Sep 18, 2022

nodejs-github-bot commented Sep 19, 2022

alan-agius4 commented Sep 20, 2022

nodejs-github-bot commented Sep 20, 2022

aduh95 commented Sep 20, 2022 • edited

legendecas commented Sep 25, 2022 • edited

juanarbol commented Oct 4, 2022

lib: don't match `sourceMappingURL` in strings #44658

lib: don't match `sourceMappingURL` in strings #44658

legendecas left a comment •

edited

legendecas Sep 15, 2022 •

edited

aduh95 commented Sep 20, 2022 •

edited

legendecas commented Sep 25, 2022 •

edited