Customizer: switch to UglifyJS2 #13151
Conversation
| @@ -264,9 +264,23 @@ window.onload = function () { // wait for load in a dumb way because B-0 | |||
| .toArray() | |||
| .join('\n') | |||
|
|
|||
| var uglify = this.UglifyJS | |||
There was a problem hiding this comment.
Extract this code into a new function perhaps?
There was a problem hiding this comment.
@cvrebert: check the refactor patch. If that's ok with you, I'll squash those two.
XhmikosR
changed the title
|
Only issue I see from this, is making debugging harder, even with sourcemaps. For some reason, including the new uglify.min.js pollutes customize.min.js. One workaround would be to include uglify.min.js separately again. |
|
Any thoughts about this? Should I go with the separate include approach? To reproduce the issue, try to debug with the first patch only. |
|
How's this make debugging harder exactly? |
|
Here is what I get without the last patch; that code is from uglify. |
|
Apparently it's FF to blame for this; Chrome works as usual. So, I skipped the last patch and this should be ready to merge. |
| @@ -11,7 +11,7 @@ window.onload = function () { // wait for load in a dumb way because B-0 | |||
| ' * Bootstrap v3.1.1 (http://getbootstrap.com)\n' + | |||
| ' * Copyright 2011-2014 Twitter, Inc.\n' + | |||
| ' * Licensed under MIT (https://github.com/twbs/bootstrap/blob/master/LICENSE)\n' + | |||
| ' */\n\n' | |||
| ' */\n' | |||
There was a problem hiding this comment.
Well, you can check the generated minified file; there's no reason for 2 newlines after the license header.
There was a problem hiding this comment.
On a side note, shouldn't we have the jquery check there too like in dist files?
There was a problem hiding this comment.
Fair enough.
And yes, we ought to...
There was a problem hiding this comment.
Done. Let me know if there's something else.
|
Looks good otherwise. |
|
I think we should have the license header and the jquery check in one place and use it in Grunt instead of duplicating those. @cvrebert feel free to take care of that after this is merged :) Otherwise, now customizer's minified build should be identical to the dist builds. |
|
Added the license header to the non minified build too. |
|
Finalized the branch. @cvrebert: thoughts about the license header and stuff? |
|
I don't have any particularly good ideas on how to avoid that duplication. It's not that much text anyway, so I personally don't have a problem with just living with the duplication. |
|
I was just asking if you agreed with the text order :P The duplication is another thing that could be a problem when bumping the copyright year or changing the license text etc. But that's for another day :) |
|
I don't really care about the order, personally. @mdo Any preference? |
|
Order? |
/* license header */
/* customizer's text */Check out this branch, |
|
@cvrebert: I think I'm gonna merge this... For the license, what do you think about combining the customizer gist text into the same comment as the license? Just using 2 new lines. |
The uglify build is generated with `uglifyjs --self -o uglify.min.js`.
|
@cvrebert: any final thoughts about the above comment? |
|
@XhmikosR Whatever you prefer is fine. |
Customizer: switch to UglifyJS2
|
I guess we can change that later then. |
/CC @fat @mdo @cvrebert