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.

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

lib: don't match sourceMappingURL in strings #44658

Closed
wants to merge 3 commits into from

Conversation

alan-agius4
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 15, 2022
@alan-agius4 alan-agius4 force-pushed the fix/js-source-maps branch 2 times, most recently from 09d29a2 to 51f24d5 Compare September 15, 2022 13:22
@cola119 cola119 added the source maps Issues and PRs related to source map support. label Sep 15, 2022
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.

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.

@aduh95 aduh95 requested a review from bcoe September 15, 2022 17:25
@aduh95 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 15, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@legendecas legendecas Sep 15, 2022

Choose a reason for hiding this comment

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

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 😴)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

//# 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
Copy link
Contributor

aduh95 commented Sep 16, 2022

@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
Copy link
Contributor Author

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

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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

@alan-agius4
Copy link
Contributor Author

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

Added comments.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2022
@nodejs-github-bot
Copy link
Collaborator

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@alan-agius4
Copy link
Contributor Author

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

Should I rebase?

@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Sep 20, 2022

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.

legendecas pushed a commit that referenced this pull request Sep 25, 2022
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
Copy link
Member

legendecas commented Sep 25, 2022

Landed in e6018e2. Thank you for your contribution!

@legendecas legendecas closed this Sep 25, 2022
@alan-agius4 alan-agius4 deleted the fix/js-source-maps branch September 25, 2022 18:43
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
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>
@RafaelGSS RafaelGSS mentioned this pull request Sep 26, 2022
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
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>
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. source maps Issues and PRs related to source map support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sourceMappingURL in string literal will cause source-maps not to load when using --enable-source-maps
6 participants