Skip to content
This repository has been archived by the owner on Nov 4, 2023. It is now read-only.

Also find sourceMappingURL= comments in the middle #1

Closed
wants to merge 1 commit into from

Conversation

netj
Copy link

@netj netj commented Jun 14, 2014

not just at the last line of the code. Strictly speaking,
sourceMappingURL= comments have to appear at the end of the file as the
last line. However, for this module to be more useful, I relaxed the
regex a bit. Ideally, we could provide an option to users whether it
should strictly stay with the specification, or pragmatically look at
the entire code for the comments.

not just at the last line of the code. Strictly speaking,
sourceMappingURL= comments have to appear at the end of the file as the
last line. However, for this module to be more useful, I relaxed the
regex a bit. Ideally, we could provide an option to users whether it
should strictly stay with the specification, or pragmatically look at
the entire code for the comments.
@lydell
Copy link
Owner

lydell commented Jun 14, 2014

for this module to be more useful

Please give an example.

Also, if this is going to be merged you’d have to update the failing test. (Tip: Always run the tests before submitting a PR!)

@netj
Copy link
Author

netj commented Jun 14, 2014

Sorry about failing tests. I'll amend the commit with updated tests.

for this module to be more useful

Please give an example.

I'm using this module to improve r.js (RequireJS optimizer) source map handling: see requirejs/r.js#692.

@lydell
Copy link
Owner

lydell commented Jun 15, 2014

I'm using this module to improve r.js (RequireJS optimizer) source map handling: see requirejs/r.js#692.

Sorry if I’m missing something obvious here, but why does that require sourceMappingURLs to be found in the middle of files?

Btw, you’re probably interested in source-map-resolve and mozilla/source-map .applySourceMap(). (And a little example code using those.)

@netj
Copy link
Author

netj commented Jun 15, 2014

No, my bad for not explaining enough, and thanks for the pointers.

The reason why I needed a relaxed regex for r.js was because while it concatenates multiple .js files (each of which can have a source map declared) it was adding semicolons as well as optional user-defined footers to the end of each file. This could be handled relatively easily, but the real problem I found was Safari's handling of sourceMappingURL= comments. It was picking up whichever comment that appears first in the JavaScript file, so unless I removed all of them in the final output of r.js, Safari kept loading the first one and never used the new source map added to the last line. In fact, the first sourceMappingURL= comment was a bogus one misplaced in the middle of a minified jQuery file, which broke my workaround of discarding and restoring the last semicolons when passing the code to source-map-url to parse the comments. That's why I gave up the workaround and simply fixed source-map-url's regex. We could say Safari (and perhaps Chrome?) is violating the standards, but if browsers start to behave this way, I would say this module could provide a pragmatic option to handle these dirty cases.

In fact, I started trying to use source-map-resolve first, and realized it was adding unnecessary complexity to r.js and the functionality provided by this module was enough to get the job done. R.js has its own abstractions for async control flow and file system, network APIs, and I didn't wanted to add too much glue code for what I mostly don't need.

Regarding SourceMapGenerator's .applySourceMap(), I don't understand what exactly it's for by looking at its interface, and how it's different from creating a SourceMapGenerator from an existing source map. How could you blindly "apply" an existing source map if the line/column numbers of each mapping in it needs careful translation for the new code? Does it simply take unions multiple source maps?

@lydell
Copy link
Owner

lydell commented Jun 15, 2014

So it’s all about removing extraneous comments that look like sourceMappingURL comments? Perhaps we should add sourceMappingURL.removeAll(code)?

@lydell lydell closed this Aug 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants