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

//# sourceMappingURL=... doesn't have to be on the last line #64

Open
nicolo-ribaudo opened this issue Jan 18, 2024 · 19 comments
Open

//# sourceMappingURL=... doesn't have to be on the last line #64

nicolo-ribaudo opened this issue Jan 18, 2024 · 19 comments

Comments

@nicolo-ribaudo
Copy link
Member

Firefox, Chrome, Safari and Node.js all seem to allow it, but the spec is not explicit about it.

@nicolo-ribaudo nicolo-ribaudo changed the title Allow a newline character be allowed after //# sourceMappingURL=... Allow a newline character after //# sourceMappingURL=... Jan 18, 2024
@nicolo-ribaudo nicolo-ribaudo changed the title Allow a newline character after //# sourceMappingURL=... Allow empty newlines after //# sourceMappingURL=... Jan 18, 2024
@jkrems
Copy link
Contributor

jkrems commented Jan 18, 2024

IIRC it goes even further and they typically allow additional lines that aren't empty after the comment? I think we have some code that depends on the ability to insert an additional marker comment after the line with the source map comment.

@nicolo-ribaudo
Copy link
Member Author

I just checked and we do the same in Babel

@jkrems
Copy link
Contributor

jkrems commented Jan 18, 2024

Potentially related: Allowing additional lines immediately means that there's ambiguity because it's no longer well defined which source map comment is supposed to win if there's multiple. I'm not sure if consumers consistently pick the last one.

@connor4312
Copy link

consumers consistently pick the last one

Consumers should, because bundles with multiple //# sourceMappingURL are pretty common as output from bundlers that bundle previously-compiled files without additional introspection (I think I saw this in Webpack)

@kamilogorek
Copy link
Member

Sentry appends //# debugId=00000000-0000-0000-0000-000000000000 after sourceMappingURL right now.
As for detection, we used to fallback to scanning last 50 (if I recall correctly?) lines of source code in case we didn't find it at the very end. Now it always scans through all lines.

@nicolo-ribaudo
Copy link
Member Author

I just checked with this test case:

"use strict";

throw new Error("Hello world!");
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiaHR0cDovL2xvY2FsaG9zdDo0NTQ1L3J1bi9pbmxpbmVfanNfc291cmNlX21hcF8yLnRzIl0sInNvdXJjZXNDb250ZW50IjpbIjErMTtcbmludGVyZmFjZSBUZXN0IHtcbiAgaGVsbG86IHN0cmluZztcbn1cblxudGhyb3cgbmV3IEVycm9yKFwiSGVsbG8gd29ybGQhXCIgYXMgdW5rbm93biBhcyBzdHJpbmcpO1xuIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiI7QUFBQSxDQUFDLEdBQUMsQ0FBQyxDQUFDO0FBS0osTUFBTSxJQUFJLEtBQUssQ0FBQyxjQUErQixDQUFDLENBQUMifQ==
console.log("AAA")
// some random comment

Firefox, Chrome, Safari, Node.js and Babel all properly detect the source map. It looks like there is indeed no restriction at all about where the //# sourceMappingURL= should be.

@nicolo-ribaudo
Copy link
Member Author

Same thing with this:

"use strict";

throw new Error("Hello world!");
; //# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiaHR0cDovL2xvY2FsaG9zdDo0NTQ1L3J1bi9pbmxpbmVfanNfc291cmNlX21hcF8yLnRzIl0sInNvdXJjZXNDb250ZW50IjpbIjErMTtcbmludGVyZmFjZSBUZXN0IHtcbiAgaGVsbG86IHN0cmluZztcbn1cblxudGhyb3cgbmV3IEVycm9yKFwiSGVsbG8gd29ybGQhXCIgYXMgdW5rbm93biBhcyBzdHJpbmcpO1xuIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiI7QUFBQSxDQUFDLEdBQUMsQ0FBQyxDQUFDO0FBS0osTUFBTSxJQUFJLEtBQUssQ0FBQyxjQUErQixDQUFDLENBQUMifQ==
console.log("AAA")
// some random comment

it looks like in practice the comment doesn't even have to be on its own line, it can be in the same line together with other code

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Jan 19, 2024

This is very fun:

"use strict";

throw new Error("Hello world!");`
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiIsInNvdXJjZXMiOlsiaHR0cDovL2xvY2FsaG9zdDo0NTQ1L3J1bi9pbmxpbmVfanNfc291cmNlX21hcF8yLnRzIl0sInNvdXJjZXNDb250ZW50IjpbIjErMTtcbmludGVyZmFjZSBUZXN0IHtcbiAgaGVsbG86IHN0cmluZztcbn1cblxudGhyb3cgbmV3IEVycm9yKFwiSGVsbG8gd29ybGQhXCIgYXMgdW5rbm93biBhcyBzdHJpbmcpO1xuIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiI7QUFBQSxDQUFDLEdBQUMsQ0FBQyxDQUFDO0FBS0osTUFBTSxJQUFJLEtBQUssQ0FBQyxjQUErQixDQUFDLENBQUMifQ==
// `

Chrome/Firefox/Safari/Babel and Node.js (but only for CJS) properly ignore the "comment", while Node.js ESM accepts it and applies the source map

@nicolo-ribaudo nicolo-ribaudo changed the title Allow empty newlines after //# sourceMappingURL=... //# sourceMappingURL=... doesn't have to be on the last line Jan 19, 2024
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Jan 19, 2024

With regards to CSS:

  • Safari, Chrome and Firefox all detect the source map in

    body .test {
      font-size: 14px;
      line-height: 28px; }
    
    body .text {
      color: green; }
      body .test::after {
        content: "";
        background-color: white; }
    
    body .test {
      padding: 5px; }
    
    a::before { /*# sourceMappingURL=sourcemap-css-absolute.map */ }
  • Safari and Chrome mistakenly detect a source map in

    body .test {
      font-size: 14px;
      line-height: 28px; }
    
    body .text {
      color: green; }
      body .test::after {
        content: "";
        background-color: white; }
    
    body .test {
      padding: 5px; }
    
    a::before { content: "/*# sourceMappingURL=sourcemap-css-absolute.map */" }

@mitsuhiko
Copy link
Contributor

I'm not sure what browsers currently do but some tools (we included) only honor a few lines from top to bottom of file for efficiency reasons. I think the question here might be if the specification can better explain what exactly the expectations are in what tools are supposed to do. It's also common for source mapping URL comments to appear more than once and which one should be picked.

My recommendation would be to request a minimum number of lines or bytes to be considered from both start and end the file and to ask tools to prioritize the last one in the file over earlier ones. (Assuming that this is the order that browers pick today)

@jkrems
Copy link
Contributor

jkrems commented Jan 20, 2024

Just want to quickly throw in that "bytes" is likely not a good unit in this context because with data URLs and sourcesContent, the source map can easily make up the majority of bytes. But restricting it to the last N lines may be viable..?

@connor4312
Copy link

connor4312 commented Jan 21, 2024

I'm not a fan of "N last lines". N is an arbitrary number; if one wants to allow all existing usages of sourceMappingURLs to be valid under the specification, then any finite N will not work. If one is okay with saying we want to tighten things up, then I think it's preferable to specify that only whitespace characters are allowed after the sourceMappingURL (note this would also require any //# sourceURL comment to be placed before the sourceMappingURL)

@mitsuhiko
Copy link
Contributor

I think we cannot tighten it up to the point where it just breaks a lot of real world stuff. I also believe forcing it to be at the end of the file will be annoying for any future specification enhancements of debug IDs or anything else that might want to have a reference there. I agree that bytes would be very tricky because of the unknown length of the comment in itself.

Maybe the most reasonable way would just be to determine the priority and to be explicit that the comment needs to be picked up from anywhere in the file.

@nicolo-ribaudo
Copy link
Member Author

One option could be, when it comes to JS / CSS, to require that the source map comment is only followed by spaces/newlines and/or other comments. This is probably good enough for all the tools I know of, and in practice it forces the comment to be "somewhere near the end".

@jkup
Copy link
Collaborator

jkup commented Jan 25, 2024

One option that came up during the TC39 TG4 features call is to change the spec to say

  • Specify that, for JavaScript, you have to parse the code (the only way to find comments reliably) and use the last sourceMappingURL they fine.
  • Specify that, for CSS, you have to use Regular Expressions to find the last "comment looking" sourceMappingURL.
  • Other languages would have to add their own specific instructions
  • Remove mention that it needs to be at the end of the file
  • Maybe recommend that producers should start and end the sourceMappingURL comment with new lines

@connor4312
Copy link

connor4312 commented Jan 25, 2024

for JavaScript, you have to parse the code (the only way to find comments reliably) and use the last sourceMappingURL they fine.

This would be a very large performance impact for VS Code's JS debugger that I would not implement. For Node.js, we scan through files on disk and parse source mapping URLs before running the code to know where to set breakpoints. This is a performance sensitive operation, and having to parse the AST for these files would make the scan at least an order of magnitude slower.

@jkup
Copy link
Collaborator

jkup commented Jan 25, 2024

@connor4312 Thanks for the quick reply! @nicolo-ribaudo pointed out that, on the other hand, if we dictate regular expressions, it would cause a slowdown for browser devtools who already have the code parsed. We should think on the best way forward.

@connor4312
Copy link

connor4312 commented Jan 25, 2024

While we should certainly bear implementation in-mind during discussion, is the extraction implementation in-scope for the sourcemap format specification?

One option could be, when it comes to JS / CSS, to require that the source map comment is only followed by spaces/newlines and/or other comments.

I like this previous suggestion, because it allows comment extensibility and parsing of comments is pretty trivial to do without having to parse the file's entire AST. It additionally enables reading from the end of the file--which it sounds like is what Armin's trying to do--something that full syntax tree parsing would also disallow. (This is something I've been wanting to do in js-debug too, but haven't gotten around to it yet.) It's also information that a comment-aware parser would have on-hand.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Feb 1, 2024

While we should certainly bear implementation in-mind during discussion, is the extraction implementation in-scope for the sourcemap format specification?

This is currently in the spec, and it has always been. We should ensure consistent behavior at least when it comes to JS and Wasm, so that users can reliably link to source maps.


EDIT: I don't know anymore if what I wrote here makes sense.

As per the last call when we discussed this, I'm trying to specify both parsing-based comment extraction and "simple" from-the-end comment extraction, but I'm finding some edge cases that we need to check how tools handle.

/* a comment */ //# sourceMapURL=foo
/* a comment */ /*# sourceMapURL=foo */
/*# sourceMapURL=foo */ // a comment
/* a comment */ /*# sourceMapURL=foo */ /* a comment */
/* a comment /*# sourceMapURL=foo */
/* a comment //# sourceMapURL=foo
// */
/* a comment
//# sourceMapURL=foo
// */
// a comment /*# sourceMapURL=foo */
// a comment //# sourceMapURL=foo
// //# sourceMapURL=foo
///# sourceMapURL=foo
//# sourceMapURL=./some/*#sourceMapURL=foo.map*/

My intuition is that all of them should be detected as a sourceMapURL comment, even if some of those looks-like-a-comment are actually just some text inside another comment.

Unless we don't actually search for comments from the end, but we iterate through the lines from the end and then in each line we look from the beginning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants