Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
We need to remove all occurrences of sourceMappingURL
and sourceURL
including the old //@
and new //#
syntax. Otherwise these source map comments are causing 404 errors after the JS files are aggregated. After JS aggregation the source maps can no longer used.
This can be done with a regex in:
- D7: drupal_build_js_cache().
- D8: JsCollectionOptimizer::optimize()
Proposed resolution
Remove sourceMappingURL
and sourceURL
from JS files.
Remaining tasks
Review and commit.
User interface changes
None.
API changes
None.
Comments
Comment #1
hass CreditAttribution: hass commentedComment #2
hass CreditAttribution: hass commentedComment #3
hass CreditAttribution: hass commentedThis patch has been inspired by http://blog.getfirebug.com/2009/08/11/give-your-eval-a-name-with-sourceurl/
Comment #5
hass CreditAttribution: hass commented*Damn* EGit bug. Reattaching.
Comment #6
hass CreditAttribution: hass commentedAnd here the D8 patch.
Comment #7
hass CreditAttribution: hass commentedComment #10
hass CreditAttribution: hass commentedNeeds test
Comment #11
hass CreditAttribution: hass commentedComment #13
hass CreditAttribution: hass commentedCannot run unit tests locally on windows. Try to find the root cause with d.o
Comment #15
hass CreditAttribution: hass commentedI have no idea what's wrong here and cannot test locally. If someone can jump in an fix the failing test it would be great. The clean code itself works well in core, it's only the tests that fail.
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedThis is causing a 404 on a vanilla install when Dev tools are enabled.
Comment #17
borisson_Going to try to get test working again
Comment #18
borisson_Test is now succesful locally
Comment #19
borisson_Comment #20
pfrenssenI would be a bit more specific in the documentation. What kind of code is removed and why?
"A javascript" and "the javascript" reads a bit weird. I would say "a javascript file" or "javascript source code".
This matches the path of the test, but it should match the namespace.
{@inheritdoc}
documentation is missing.Aren't we just comparing files that are already present in core?Edit: I was mistaken, it is used in the test itself.$this->optimizer
seems to be unused in the test.Coding standards: some of the test files do not end in a newline.
Comment #21
nod_For the newline comment it's usual that minified files for vendor libraries do not end with a newline. Our tests should include at least a minified file without newline to make sure it works.
Comment #22
hass CreditAttribution: hass commentedThanks for fixing the tests.
We need to extend the case I think. I heard that the same comments could be inside CSS. Additionally to this we could move the charset removal from CSS files to the clean function and add the clean function to the interface.
EDIT:
/*# sourceMappingURL=<url> */
will be removed by our CSS compressor. However it would be cleaner to add the charset at least to the CSS cleanupComment #23
hass CreditAttribution: hass commentedThat patch is what I was thinking about.
The review from #20 seems mostly invalid.
Comment #25
pfrenssen@Hass, if you copy incomplete or incorrect code from another test that doesn't mean that it is acceptable in a new patch. Regarding #3 - the namespace mentioned in the @file docblock is different from the actual namespace used, so it is clearly incorrect. #4 has missing documentation which is against standards.
I agree on #5 and #6 these can be disregarded. I have no strong feelings about #1 but this documentation is so vague that it raises more questions than answers in my mind.
Comment #26
cilefen CreditAttribution: cilefen commentedThe 404's are not a production error because they only occur in the context of browser devtools, as I have demonstrated in #1341792-77: [meta] Ship minified versions of external JavaScript libraries.
Comment #27
cilefen CreditAttribution: cilefen commentedComment #28
hass CreditAttribution: hass commentedNevertheless we should remove these on aggregation as map files will never fit for the aggregated code.
Comment #29
cutesquirrel CreditAttribution: cutesquirrel commentedComment #30
cutesquirrel CreditAttribution: cutesquirrel commentedHi,
the last failed test seems only due to a "@charset" : since the patch, the charset are not anymore removed (normal).
So i've added it to optimized css version, and replace '' by "".
Hope this helps
Comment #31
hass CreditAttribution: hass commentedDebug?
This is wrong. All charsets need to be removed on CSS optimization.
Comment #32
cutesquirrel CreditAttribution: cutesquirrel commentedOups, it was a bit too late in the night... my mind wasn't effective !
Now it should be ok :)
Thanks @hass !
Comment #33
hass CreditAttribution: hass commentedGreat. :-) What about the commented verbose()?
Comment #34
rteijeiro CreditAttribution: rteijeiro commented@hass, I can't find the commented verbose you mention here. The patch looks good to me. Still applies and tests are ok in local environment. It's RTBC?
Comment #35
pfrenssenThis needs a bit of clean-up.
Data types are missing for $contents and the return value.
Comment exceeds 80 characters.
Missing data types.
The namespace in the docblock doesn't match the namespace in the class.
This method doesn't have documentation, you can simply add {@inheritdoc} here.
It would be nice if you could add an @see that refers to the method that uses this data.
This should also document the return value. Usually people just say that it returns "an array of test data".
This sentence could use some love. What about "Tests cleaning of a JS asset group containing 'type' => 'file'"?
Anonymous functions should have a space between "function" and the opening parenthesis. Or is this space stripped as part of the minifying process?
This is in all the minified files.
Comment #36
rteijeiro CreditAttribution: rteijeiro commentedFixed points 1,2,3,4,5,6 and 7 in #35. Not sure about point 8. Those JS files are manually created for the tests so no minified.
Comment #37
pfrenssenGreat, thanks Ruben! RTBC if the bot comes back green.
Comment #38
hass CreditAttribution: hass commentedBetween #30 and #32 you removed the CssOptimizerUnitTest and therefore the verbose!? But now we have no unit test for this any longer.
Comment #39
hass CreditAttribution: hass commentedComment #40
borisson_The CssOptimizerUnitTest is still in source code, it got removed from the patch because the code that was used for testing was removed.
All this code does is dump the css assets to the /tmp folder to manually test if the files are correctly optimized. The verbose method in there is a reference to the phpunit verbose method. (https://phpunit.de/manual/current/en/textui.html).
Comment #41
webchickCan someone double-check this patch is still valid, despite #2473837: Use minified jQuery once just got committed, which added a classmap?
Comment #42
aspilicious CreditAttribution: aspilicious commentedRerun the tests and they all passed, they didn't rely on our JS libraries itself.
So we are safe.
Comment #43
webchickGreat.
That foo vs. foo_old stuff was a little funny, but from a bit of Googling I see that this is basically an external standard outside of our control.
Committed and pushed to 8.0.x. Thanks!
Comment #44
aspilicious CreditAttribution: aspilicious commentedCan you triple check? We don't see anything in the logs.
Comment #45
webchickAHEM! Sorry about that. :)
Comment #47
hass CreditAttribution: hass commentedThanks for committing.
Now we need to review patch #5 for D7.
Comment #48
yuriy.babenko CreditAttribution: yuriy.babenko commentedI re-rolled patch in #5 (as-is) against 7.37. Works well on this end.
Comment #49
star-szrThanks @yuriy.babenko, patches need peer review though.
Comment #50
netbek CreditAttribution: netbek commentedThe patch in #48 unfortunately breaks lodash because the regex matches
//# sourceURL=
which isn't at the end of the minified file. It's part of the lodashtemplate
function: https://github.com/lodash/lodash/blob/b9d1a938c3770ed09589ce93d75e29afa1...Is it common for source maps URLs to start on a new line? If so, then adding a start-of-line anchor to the pattern might work. Or should it check that matches are at the end of the file?
Comment #51
netbek CreditAttribution: netbek commentedThinking this through some more, a simple start-of-line check isn't enough. It should check if the source map comment is at the end of each file that's aggregated. I'll try to post a patch when time allows.
Comment #53
deanflory CreditAttribution: deanflory as a volunteer commentedThis was still an issue for D7 right? Just came across it.
Comment #54
hass CreditAttribution: hass commentedYes
Comment #55
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commentedD7 version from #5 works good for me.
But I wonder why we are removing this info, instead of setting up a proper path to .map files to make them work?
Comment #58
deanflory CreditAttribution: deanflory as a volunteer commented#5 still applies and appears to fix the error in D7.50
Comment #59
hass CreditAttribution: hass commentedComment #60
stefan.r CreditAttribution: stefan.r commentedThe regex is identical to the one in D8 so this seems fine.
In the reroll an extra "// Prefix filename to prevent blocking by firewalls which reject files" slipped in though, can anyone post a proper reroll?
Comment #61
stefan.r CreditAttribution: stefan.r commentedComment #62
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedRerolling patch from #48. Removed comment "// Prefix filename to prevent blocking by firewalls which reject files".
Comment #66
deanflory CreditAttribution: deanflory as a volunteer commentedSo did #62 pass or fail? What is the appropriate patch for D7.54? #5? #62?
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedComment #68
David_Rothstein CreditAttribution: David_Rothstein commentedI moved it back to needs review since the tests were passing. However it looks like #50 and #51 still need discussion?
Comment #69
pandaski CreditAttribution: pandaski commentedThis line is matching the D8 snippet.
This is RTBC to D7
Comment #70
minakshiPh CreditAttribution: minakshiPh at TATA Consultancy Services for Pfizer, Inc. commentedHi,
The patch in #62 resolves my error coming for another module https://www.drupal.org/project/janrain_capture/issues/3145691#comment-13730221 so marking it RTBC.
+RTBC
Thanks,
minakshiPh
Comment #71
cyb_tachyon CreditAttribution: cyb_tachyon as a volunteer and at Red Hat commentedNoting here that this doesn't seem to properly move webpack URI's and can break aggregated third party JS.
Example:
//# sourceURL=webpack://Quill/
is stripped from//# sourceURL=webpack://Quill/./assets/icons/align-left.svg?
- it seems to stop parsing at the./
character when a '.' is a valid part of a webpack URI.Without agg:
With agg:
Comment #72
mcdruidConfirmed (per #69) that the line added in #62 is exactly the same as what's in D9:
https://git.drupalcode.org/project/drupal/-/blob/9.1.5/core/lib/Drupal/C...
Comment #74
mcdruidPer #68, if #50 / #51 is still a problem that's the case in D8/9 too and would require a follow-up for D7/8/9.
Thanks!
Comment #76
filipetakanap CreditAttribution: filipetakanap commentedye, i still have this error for a long time even after updates... D9