lib: don't match `sourceMappingURL` in strings by alan-agius4 · Pull Request #44658 · nodejs/node · GitHub

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

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.

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

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.

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

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

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

Added comments.

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

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

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

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

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

Should I rebase?

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

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>

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>

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

Comment thread lib/internal/source_map/source_map_cache.js Outdated

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

Comment thread lib/internal/source_map/source_map_cache.js Outdated

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

Comment thread lib/internal/source_map/source_map_cache.js Outdated

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 outstanding review comments, 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

Comment thread test/message/source_map_sourcemapping_url_string.out Outdated

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

Closed

2 tasks

lib: don't match `sourceMappingURL` in strings by alan-agius4 · Pull Request #44658 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

lib: don't match sourceMappingURL in strings#44658alan-agius4 wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom alan-agius4:fix/js-source-mapsalan-agius4/node:fix/js-source-mapsCopy head branch name to clipboard

Conversation

alan-agius4 commented Sep 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

legendecas left a comment • 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!

nodejs-github-bot commented Sep 15, 2022

Uh oh!

legendecas Sep 15, 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!

alan-agius4 Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Sep 16, 2022

Uh oh!

alan-agius4 commented Sep 16, 2022

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

alan-agius4 commented Sep 16, 2022

Uh oh!

nodejs-github-bot commented Sep 18, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 18, 2022

Uh oh!

nodejs-github-bot commented Sep 19, 2022

Uh oh!

alan-agius4 commented Sep 20, 2022

Uh oh!

nodejs-github-bot commented Sep 20, 2022

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

juanarbol commented Oct 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lib: don't match sourceMappingURL in strings#44658
alan-agius4 wants to merge 3 commits into
nodejs:mainfrom
alan-agius4:fix/js-source-maps

legendecas left a comment •
edited
Loading

legendecas Sep 15, 2022 •
edited
Loading

aduh95 commented Sep 20, 2022 •
edited
Loading

legendecas commented Sep 25, 2022 •
edited
Loading