| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Add concat, compile and minify Grunt tasks #5776
Conversation
| var promise, e = new Error(), stackDepth = e.stack.split("\n").length; | ||
|
|
||
| // Use E for Error so that uglify doesn't change this to simply Error() | ||
| var promise, E = Error, e = new E(), stackDepth = e.stack.split("\n").length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me think twice about minification. Without this, uglify essentially removes the new keyword, causing the error stack length to increase by 1, throwing off our logic here for detecting when a command is fired while Brackets is at a breakpoint. I vaguely remember @peterflynn having concerns just like this long long ago when it comes to JavaScript minification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you measured the amount of time saved by minification vs. simply concatenating the files? I would guess that the big win is in concatenation, but I don't know for sure.
Regardless, if we want Brackets to work reasonably well in the browser, pursuing minification is likely worthwhile and as long as we don't need too many hacks like this, it seems reasonable.
(tangential aside: I've also wondered about us using Closure Compiler on our code, more for type checking than minification. It would be interesting to know if Closure Compiler does this same optimization. Of course, Closure Compiler is written in Java which is something of a drag.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, in my initial report I compared compiled LESS as the baseline, then added JS concat/minify. I can go back and compare concat and concat+minify separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably fine as is. We're going to want to properly support minified files because concat+minify+gzip is going to be less for the browser than just concat+gzip, I think.
ghost
assigned
…dev/dist build.
Conflicts: src/index.html src/main.js src/utils/ExtensionLoader.js
|
@dangoor I added source maps, but only for everything in our RequireJS root. Thirtparty libs are concatenated, but not minified. I hadn't tried out source maps in Chrome until now, but it seemed to work as advertised. The original files show up on a different node in the source tree labeled "(no domain)". |
|
Awesome. Sounds like a good time to try this out and get to familiarizing myself with it. |
fix build task sequence do not optimize config.json
|
Following your steps in the description, grunt fails because it can't find a build-config task: |
|
Just to make sure I know what we're going for here: the intention is that we continue developing Brackets as we have been, but when we go to ship it, we'll be shipping the bits from |
|
I merged master in (fixing the conflict in .gitignore). I also pulled down your brackets-shell branch, and merged master in there. I removed the reference to build-config so that I could run |
|
Might be a conflict with the build.prop merge for the release branch. I'll take a look. Thanks! |
|
Yes, we can continue developing and running brackets without a build step in our development environment. We will only pick up the dist folder in a non-dev-env or when explicitly chosen. |
Conflicts: .gitignore
|
@dangoor can you try again now that I fixed the merge conflict. That should work, but if it doesn't you can also try the |
|
@jasonsanjose Yep, it works now (just using standard brackets-shell)! It did feel zippy starting up. |
|
|
||
| <link rel="stylesheet" type="text/css" href="thirdparty/CodeMirror2/lib/codemirror.css"> | ||
| <!--(if target dev)><!--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow. that's pretty subtle... the dev target isn't commented out but the dist one is. Luckily, we don't have bunches of code that looks like this!
|
Looks good. I'm going to familiarize myself with a couple of the grunt bits here that I'm unfamiliar with and do some testing in the morning. It will likely be ready for merging then. I do like how this preserves our current development experience and provides a way to speed up startup time for most users. This does mean that we'll need to be sure we test our builds well given that they will be a bit more different from our development environment than they are today. |
|
@dangoor let me know if I can help clarify anything or add comments. |
| 'htmlmin', | ||
| 'requirejs', | ||
| 'concat', | ||
| /*'cssmin',*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where are these two here but commented out? should they just be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they can be removed. I'll follow up with another pull.
|
I don't think any additional comments are needed... Making sense of a Gruntfile requires understanding how Grunt itself works and also what the tasks do. This Gruntfile does quite a bit without a lot of code which is good but also a bit more opaque. Everything looks fine from the testing I did this morning. |
Add concat, compile and minify Grunt tasks
To test this branch:
npm installto install the new grunt tasksgrunt buildto compilesrcto a newdistfolderSHIFTkey when opening a new Brackets window and selectdist/index.htmlNotes:
index.htmlto optionally includebrackets.lessvs compiledbrackets.cssviagrunt lessgrunt buildtask to compile less and concat thirdparty libs, compile, minify and generate source map for brackets src/*/.jsmain.jsto be compatible with r.js.gitignoreSource Maps:
On my Mac,
document.readytime is reduced from ~2.5s to ~1.4s, largely due to the compiled LESS code. Adding JavaScript minification bringsdocument.readytime down to ~1.0s.On Windows, we I go from ~3.8s to ~2.3s with LESS compiled only. ~2.0s with minification.
Still need to update brackets-shell packaging tasks.
This setup requires developers to install node and grunt to compilebrackets.lesstobrackets.css. The other option would be to commit the compiledbrackets.cssto the repo. This would mean adding a 2nd generated file that I know of in addition to/src/config.json. We didn't pull the trigger on requiring node and grunt for contributors in the past so that the barrier to entry would stay low. Also, the payoff prior to now was not that great. I think we should reconsider changing the setup requirements for developers if we're averse to checking in compiled assets.I have an old open pull request to port the dev env setup to Grunt here #4577.UPDATE: This setup allows the current dev env setup to remain intact as far as dynamic LESS compilation. My prior assertion about requiring node+grunt no longer applies, and the case to require these tools for all developers is not as strong.
Open Questions
brackets.lessand whether or not it is desirable to so. --- ANSWER: No, trying to access variables and mixins does not work. Through ourExtensionUtilsmodule, each .less file is compiled with it's own parser. Variables and mixins are only known to the parser created at startup time.