Add concat, compile and minify Grunt tasks #5776

jasonsanjose · 2013-10-31T02:07:14Z

To test this branch:

Run npm install to install the new grunt tasks
Run grunt build to compile src to a new dist folder
Hold the SHIFT key when opening a new Brackets window and select dist/index.html

Notes:

Change index.html to optionally include brackets.less vs compiled brackets.css via grunt less
Add new grunt build task to compile less and concat thirdparty libs, compile, minify and generate source map for brackets src/*/.js
Change main.js to be compatible with r.js
Minor changes to extensions and ShellAPI to account for new compile steps
Update .gitignore

Source Maps:

JavaScript source maps are supported as of brackets-shell for sprint 34
LESS source maps are not supported in brackets-shell (requires CEF with Chrome 30+). Source maps are generated and can be tested in Chrome. Verified on Chrome 31.

On my Mac, document.ready time is reduced from ~2.5s to ~1.4s, largely due to the compiled LESS code. Adding JavaScript minification brings document.ready time 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 compile brackets.less to brackets.css. The other option would be to commit the compiled brackets.css to 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

Determine if extensions could rely on LESS mixins defined in brackets.less and whether or not it is desirable to so. --- ANSWER: No, trying to access variables and mixins does not work. Through our ExtensionUtils module, each .less file is compiled with it's own parser. Variables and mixins are only known to the parser created at startup time.

jasonsanjose · 2013-10-31T16:47:48Z

src/utils/ShellAPI.js

-        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;


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.

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.)

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.

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.

…dev/dist build.

Conflicts: src/index.html src/main.js src/utils/ExtensionLoader.js

jasonsanjose · 2013-11-15T00:49:00Z

@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)".

dangoor · 2013-11-15T01:57:45Z

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

dangoor · 2013-12-04T14:33:17Z

Following your steps in the description, grunt fails because it can't find a build-config task:

grunt build
Warning: Task "build-config" not found. Use --force to continue.

Aborted due to warnings.

dangoor · 2013-12-04T14:43:36Z

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 dist?

dangoor · 2013-12-04T14:50:31Z

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 grunt build. I had what appeared to be an acceptable dist directory. I built brackets-shell, fired it up and pointed it at dist/index.html. It's failing somewhere in startup for me (nothing in the window, menus aren't set up).

jasonsanjose · 2013-12-04T16:37:04Z

Might be a conflict with the build.prop merge for the release branch. I'll take a look. Thanks!

jasonsanjose · 2013-12-04T16:39:04Z

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

jasonsanjose · 2013-12-04T16:55:48Z

@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/minify branch in brackets-shell and run grunt full-build there. Assuming you're on the jasonsanjose/yeoman branch in www, it will build the shell and copy www into the staging folder.

dangoor · 2013-12-04T20:17:40Z

@jasonsanjose Yep, it works now (just using standard brackets-shell)! It did feel zippy starting up.

dangoor · 2013-12-04T20:47:13Z

src/index.html


+    <link rel="stylesheet" type="text/css" href="thirdparty/CodeMirror2/lib/codemirror.css">
+    <!--(if target dev)><!-->


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!

dangoor · 2013-12-04T21:01:19Z

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.

jasonsanjose · 2013-12-04T21:10:48Z

@dangoor let me know if I can help clarify anything or add comments.

dangoor · 2013-12-05T14:56:46Z

Gruntfile.js

+        'htmlmin',
+        'requirejs',
+        'concat',
+        /*'cssmin',*/


where are these two here but commented out? should they just be removed?

Yes, they can be removed. I'll follow up with another pull.

dangoor · 2013-12-05T15:01:08Z

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

jasonsanjose added 2 commits October 29, 2013 20:09

inital pass at compiling less files

86c003c

minify javascript

3b9c5c0

jasonsanjose reviewed Oct 31, 2013
View reviewed changes

cleanup copy task

fabd7b0

ghost assigned dangoor Nov 1, 2013

add grunt-targethtml to conditionally use brackets less/css based on …

f51764b

…dev/dist build.

redmunds mentioned this pull request Nov 11, 2013

Faster brackets load time #5938

Closed

jasonsanjose added 5 commits November 14, 2013 11:49

Merge branch 'master' into jasonsanjose/yeoman

342d42d

Conflicts: src/index.html src/main.js src/utils/ExtensionLoader.js

Missed a merge conflict in ExtensionLoader

b68d69c

add sourcemaps for requirejs

a82ae3d

restore default task. new build task for compiling, concat, etc.

291d664

Merge remote-tracking branch 'origin/master' into jasonsanjose/yeoman

7fd497f

Add source maps for LESS. Requires a CEF upgrade to Chrome 30+.

3bd69db

jasonsanjose mentioned this pull request Nov 21, 2013

change copy:www to use minified source adobe/brackets-shell#385

Merged

jasonsanjose added 2 commits November 27, 2013 12:01

Merge branch 'master' into jasonsanjose/yeoman

0332a1f

add config.json to build task

fe09112

fix build task sequence do not optimize config.json

This was referenced Dec 3, 2013

Changes to build.prop file to support Jenkins jobs #6076

Merged

Share HTML templates, resources for localized getting started projects #6170

Open

Merge branch 'master' into jasonsanjose/yeoman

ef1bac2

Conflicts: .gitignore

dangoor reviewed Dec 4, 2013
View reviewed changes

dangoor reviewed Dec 5, 2013
View reviewed changes

dangoor added a commit that referenced this pull request Dec 5, 2013

Merge pull request #5776 from adobe/jasonsanjose/yeoman

39bac0b

Add concat, compile and minify Grunt tasks

dangoor merged commit 39bac0b into master Dec 5, 2013

dangoor deleted the jasonsanjose/yeoman branch December 5, 2013 15:02

le717 mentioned this pull request Dec 21, 2013

Question: Where is build target defined? #6296

Closed


		<link rel="stylesheet" type="text/css" href="thirdparty/CodeMirror2/lib/codemirror.css">
		<!--(if target dev)><!-->

Add concat, compile and minify Grunt tasks #5776

Add concat, compile and minify Grunt tasks #5776

Uh oh!

Conversation

jasonsanjose commented Oct 31, 2013

Open Questions

Uh oh!

jasonsanjose Oct 31, 2013

Choose a reason for hiding this comment

Uh oh!

dangoor Dec 4, 2013

Choose a reason for hiding this comment

Uh oh!

jasonsanjose Dec 5, 2013

Choose a reason for hiding this comment

Uh oh!

dangoor Dec 5, 2013

Choose a reason for hiding this comment

Uh oh!

jasonsanjose commented Nov 15, 2013

Uh oh!

dangoor commented Nov 15, 2013

Uh oh!

dangoor commented Dec 4, 2013

Uh oh!

dangoor commented Dec 4, 2013

Uh oh!

dangoor commented Dec 4, 2013

Uh oh!

jasonsanjose commented Dec 4, 2013

Uh oh!

jasonsanjose commented Dec 4, 2013

Uh oh!

jasonsanjose commented Dec 4, 2013

Uh oh!

dangoor commented Dec 4, 2013

Uh oh!

dangoor Dec 4, 2013

Choose a reason for hiding this comment

Uh oh!

dangoor commented Dec 4, 2013

Uh oh!

jasonsanjose commented Dec 4, 2013

Uh oh!

dangoor Dec 5, 2013

Choose a reason for hiding this comment

Uh oh!

jasonsanjose Dec 5, 2013

Choose a reason for hiding this comment

Uh oh!

dangoor commented Dec 5, 2013

Uh oh!

Uh oh!