Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Add concat, compile and minify Grunt tasks #5776

Merged
merged 13 commits into from
Dec 5, 2013
Merged

Add concat, compile and minify Grunt tasks #5776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
86c003c
inital pass at compiling less files
jasonsanjose Oct 30, 2013
3b9c5c0
minify javascript
jasonsanjose Oct 31, 2013
fabd7b0
cleanup copy task
jasonsanjose Oct 31, 2013
f51764b
add grunt-targethtml to conditionally use brackets less/css based on …
jasonsanjose Nov 1, 2013
342d42d
Merge branch 'master' into jasonsanjose/yeoman
jasonsanjose Nov 14, 2013
b68d69c
Missed a merge conflict in ExtensionLoader
jasonsanjose Nov 14, 2013
a82ae3d
add sourcemaps for requirejs
jasonsanjose Nov 14, 2013
291d664
restore default task. new build task for compiling, concat, etc.
jasonsanjose Nov 15, 2013
7fd497f
Merge remote-tracking branch 'origin/master' into jasonsanjose/yeoman
jasonsanjose Nov 15, 2013
3bd69db
Add source maps for LESS. Requires a CEF upgrade to Chrome 30+.
jasonsanjose Nov 15, 2013
0332a1f
Merge branch 'master' into jasonsanjose/yeoman
jasonsanjose Nov 27, 2013
fe09112
add config.json to build task
jasonsanjose Dec 2, 2013
ef1bac2
Merge branch 'master' into jasonsanjose/yeoman
jasonsanjose Dec 4, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
Thumbs.db
src/brackets.css
src/brackets.min.css

# ignore jenkins build info
/build.prop
Expand All @@ -11,19 +9,22 @@ src/brackets.min.css

# ignore compiled files
/dist
/src/.index.html
/src/styles/brackets.min.css
/src/styles/brackets.min.css.map

# ignore everything in the dev extension directory EXCEPT the README
# (so that the directory is non-empty and can be in git)
src/extensions/dev/*
!src/extensions/dev/README
/src/extensions/dev/*
!/src/extensions/dev/README

src/extensions/disabled
/src/extensions/disabled

#OSX .DS_Store files
.DS_Store

# unit test working directory
test/results
/test/results

# Netbeans
/nbproject
Expand All @@ -32,4 +33,4 @@ test/results
.idea

# Files that can be automatically downloaded that we don't want to ship with our builds
src/extensibility/node/node_modules/request/tests/
/src/extensibility/node/node_modules/request/tests/
168 changes: 160 additions & 8 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,152 @@
module.exports = function (grunt) {
'use strict';

// load dependencies
require('load-grunt-tasks')(grunt, {pattern: ['grunt-contrib-*', 'grunt-targethtml', 'grunt-usemin']});
grunt.loadTasks('tasks');

var common = require("./tasks/lib/common")(grunt);

// Project configuration.
grunt.initConfig({
pkg : grunt.file.readJSON("package.json"),
clean: {
dist: {
files: [{
dot: true,
src: [
'dist',
'src/.index.html',
'src/styles/brackets.css'
]
}]
}
},
copy: {
dist: {
files: [
{
'dist/index.html': 'src/.index.html'
},
/* static files */
{
expand: true,
dest: 'dist/',
cwd: 'src/',
src: [
'nls/{,*/}*.js',
'xorigin.js',
'dependencies.js',
'thirdparty/requirejs/require.js',
'LiveDevelopment/launch.html'
]
},
/* extensions and CodeMirror modes */
{
expand: true,
dest: 'dist/',
cwd: 'src/',
src: [
'extensibility/**/*',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need this for finding the extension manager node domain.

'!extensions/default/*/unittest-files/**/*',
'!extensions/default/*/unittests.js',
'extensions/default/*/**/*',
'thirdparty/CodeMirror2/addon/{,*/}*',
'thirdparty/CodeMirror2/keymap/{,*/}*',
'thirdparty/CodeMirror2/lib/{,*/}*',
'thirdparty/CodeMirror2/mode/{,*/}*',
'thirdparty/CodeMirror2/theme/{,*/}*',
'thirdparty/i18n/*.js',
'thirdparty/text/*.js'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need these for ExtensionLoader when creating new require contexts.

]
},
/* styles, fonts and images */
{
expand: true,
dest: 'dist/styles',
cwd: 'src/styles',
src: ['jsTreeTheme.css', 'fonts/{,*/}*.*', 'images/*', 'brackets.min.css*']
}
]
}
},
less: {
dist: {
files: {
"src/styles/brackets.min.css": "src/styles/brackets.less"
},
options: {
compress: true,
sourceMap: true,
sourceMapFilename: 'src/styles/brackets.min.css.map',
outputSourceFiles: true,
sourceMapRootpath: '',
sourceMapBasepath: 'src/styles'
}
}
},
requirejs: {
dist: {
// Options: https://github.com/jrburke/r.js/blob/master/build/example.build.js
options: {
// `name` and `out` is set by grunt-usemin
baseUrl: 'src',
optimize: 'uglify2',
// TODO: Figure out how to make sourcemaps work with grunt-usemin
// https://github.com/yeoman/grunt-usemin/issues/30
generateSourceMaps: true,
useSourceUrl: true,
// required to support SourceMaps
// http://requirejs.org/docs/errors.html#sourcemapcomments
preserveLicenseComments: false,
useStrict: true,
// Disable closure, we want define/require to be globals
wrap: false,
exclude: ["text!config.json"],
uglify2: {} // https://github.com/mishoo/UglifyJS2
}
}
},
targethtml: {
dist: {
files: {
'src/.index.html': 'src/index.html'
}
}
},
useminPrepare: {
options: {
dest: 'dist'
},
html: 'src/.index.html'
},
usemin: {
options: {
dirs: ['dist']
},
html: ['dist/{,*/}*.html']
},
htmlmin: {
dist: {
options: {
/*removeCommentsFromCDATA: true,
// https://github.com/yeoman/grunt-usemin/issues/44
//collapseWhitespace: true,
collapseBooleanAttributes: true,
removeAttributeQuotes: true,
removeRedundantAttributes: true,
useShortDoctype: true,
removeEmptyAttributes: true,
removeOptionalTags: true*/
},
files: [{
expand: true,
cwd: 'src',
src: '*.html',
dest: 'dist'
}]
}
},
meta : {
src : [
'src/**/*.js',
Expand Down Expand Up @@ -144,16 +285,9 @@ module.exports = function (grunt) {
linux: "<%= shell.repo %>/installer/linux/debian/package-root/opt/brackets/brackets"
}
});

// load dependencies
grunt.loadTasks('tasks');
grunt.loadNpmTasks('grunt-contrib-jasmine');
grunt.loadNpmTasks('grunt-contrib-jshint');
grunt.loadNpmTasks('grunt-contrib-watch');
grunt.loadNpmTasks('grunt-jasmine-node');

// task: install
grunt.registerTask('install', ['write-config']);
grunt.registerTask('install', ['write-config', 'less']);

// task: test
grunt.registerTask('test', ['jshint:all', 'jasmine']);
Expand All @@ -163,6 +297,24 @@ module.exports = function (grunt) {
// Update sprint number in package.json and rewrite src/config.json
grunt.registerTask('set-sprint', ['update-sprint-number', 'write-config']);

// task: build
grunt.registerTask('build', [
'jshint:src',
'jasmine',
'clean',
'less',
'targethtml',
'useminPrepare',
'htmlmin',
'requirejs',
'concat',
/*'cssmin',*/
Copy link
Contributor

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?

Copy link
Member Author

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.

/*'uglify',*/
'copy',
'usemin',
'build-config'
]);

// Default task.
grunt.registerTask('default', ['test']);
};
12 changes: 12 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,18 @@
"grunt-contrib-watch": "0.3.1",
"grunt-contrib-jasmine": "0.4.2",
"grunt-template-jasmine-requirejs": "0.1.0",
"grunt-contrib-cssmin": "0.6.0",
"grunt-contrib-clean": "0.4.1",
"grunt-contrib-copy": "0.4.1",
"grunt-contrib-htmlmin": "0.1.3",
"grunt-contrib-less": "0.8.2",
"grunt-contrib-requirejs": "0.4.1",
"grunt-contrib-uglify": "0.2.0",
"grunt-contrib-concat": "0.3.0",
"grunt-contrib-watch": "0.4.3",
"grunt-targethtml": "0.2.6",
"grunt-usemin": "0.1.11",
"load-grunt-tasks": "0.2.0",
"q": "0.9.2",
"jshint": "2.1.4"
},
Expand Down
1 change: 0 additions & 1 deletion src/brackets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,5 @@ define(function (require, exports, module) {
// Dispatch htmlReady event
_beforeHTMLReady();
AppInit._dispatchReady(AppInit.HTML_READY);

$(window.document).ready(_onReady);
});
13 changes: 12 additions & 1 deletion src/config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,20 @@
"phantomjs": "1.9.0-1",
"grunt-lib-phantomjs": "0.3.0",
"grunt-contrib-jshint": "0.6.0",
"grunt-contrib-watch": "0.3.1",
"grunt-contrib-watch": "0.4.3",
"grunt-contrib-jasmine": "0.4.2",
"grunt-template-jasmine-requirejs": "0.1.0",
"grunt-contrib-cssmin": "0.6.0",
"grunt-contrib-clean": "0.4.1",
"grunt-contrib-copy": "0.4.1",
"grunt-contrib-htmlmin": "0.1.3",
"grunt-contrib-less": "0.8.2",
"grunt-contrib-requirejs": "0.4.1",
"grunt-contrib-uglify": "0.2.0",
"grunt-contrib-concat": "0.3.0",
"grunt-targethtml": "0.2.6",
"grunt-usemin": "0.1.11",
"load-grunt-tasks": "0.2.0",
"q": "0.9.2",
"jshint": "2.1.4"
},
Expand Down
23 changes: 15 additions & 8 deletions src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,22 @@
<!-- NOTE: All scripts must be external for Chrome App support: http://developer.chrome.com/apps/app_csp.html -->

<!-- Warn about failed cross origin requests in Chrome -->
<script type="application/javascript" src="xorigin.js">
</script>

<!-- TODO (Issue #278): switch between runtime LESS compilation in dev mode and precompiled version -->
<link rel="stylesheet/less" href="styles/brackets.less">
<script src="thirdparty/less-1.4.2.min.js"></script>
<script src="thirdparty/mustache/mustache.js"></script>
<!-- <link rel="stylesheet" href="brackets.min.css"> -->
<script type="application/javascript" src="xorigin.js"></script>

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

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!

<link rel="stylesheet" type="text/less" href="styles/brackets.less">
<!--<!(endif)-->
<!--(if target dist)>
<link rel="stylesheet" type="text/css" href="styles/brackets.min.css">
<!--<!(endif)-->

<!-- JavaScript -->

<!-- Pre-load third party scripts that cannot be async loaded. -->
<!-- build:js thirdparty/thirdparty.min.js -->
<script src="thirdparty/less-1.4.2.min.js"></script>
<script src="thirdparty/mustache/mustache.js"></script>
<script src="thirdparty/jquery-2.0.1.min.js"></script>
<script src="thirdparty/CodeMirror2/lib/codemirror.js"></script>
<script src="thirdparty/CodeMirror2/addon/fold/xml-fold.js"></script>
Expand All @@ -59,6 +63,7 @@

<!-- JS for CodeMirror search support -->
<script src="thirdparty/CodeMirror2/addon/search/searchcursor.js"></script>
<!-- endbuild -->

</head>
<body>
Expand All @@ -70,7 +75,9 @@
-->

<!-- All other scripts are loaded through require. -->
<!-- build:js main.js -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see the change caused by the build:js for thirdparty.min.js, but the only difference I see here is that the attributes switched order... otherwise, the script tag seems exactly the same. What should this be doing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sets up configuration used by the requirejs (r.js) task. You're right that it sort of looks like a no-op though. Anyhow, the config is printed to the terminal:

  requirejs:
  { dist: 
   { options: 
      { baseUrl: 'src',
        optimize: 'uglify2',
        generateSourceMaps: true,
        useSourceUrl: true,
        preserveLicenseComments: false,
        useStrict: true,
        wrap: false,
        exclude: [ 'text!config.json' ],
        uglify2: {},
        name: 'main',
        out: 'dist/main.js',
        mainConfigFile: 'src/main.js' } } }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's like magic voodoo. There's nothing in the Gruntfile that runs r.js (looks like usemin takes care of it).

<script src="thirdparty/requirejs/require.js" data-main="main"></script>
<!-- endbuild -->

<!-- Verify that all dependencies are loaded. -->
<script src="dependencies.js"></script>
Expand Down
16 changes: 10 additions & 6 deletions src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,16 @@ require.config({
// The file system implementation. Change this value to use different
// implementations (e.g. cloud-based storage).
"fileSystemImpl" : "filesystem/impls/appshell/AppshellFileSystem"
},
// Use custom brackets property until CEF sets the correct navigator.language
// NOTE: When we change to navigator.language here, we also should change to
// navigator.language in ExtensionLoader (when making require contexts for each
// extension).
}
});

// hack for r.js optimization, move locale to another config call

// Use custom brackets property until CEF sets the correct navigator.language
// NOTE: When we change to navigator.language here, we also should change to
// navigator.language in ExtensionLoader (when making require contexts for each
// extension).
require.config({
locale: window.localStorage.getItem("locale") || (typeof (brackets) !== "undefined" ? brackets.app.language : navigator.language)
});

Expand All @@ -51,4 +56,3 @@ define(function (require, exports, module) {
// Load the brackets module. This is a self-running module that loads and runs the entire application.
require("brackets");
});

6 changes: 0 additions & 6 deletions src/styles/brackets_shared.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,6 @@
* processing order.
*/

/* CSS imports */

// CodeMirror
// Must wrap in quotes to avoid issue #1015 (true for all relative URLs to non-LESS files)
@import "../thirdparty/CodeMirror2/lib/codemirror.css";

/* LESS imports */

// Bootstrap @ v.2.3.1
Expand Down
6 changes: 5 additions & 1 deletion src/utils/ExtensionLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ define(function (require, exports, module) {
* C:\Users\<user>\AppData\Roaming\Brackets\extensions\user on windows.
*/
function getUserExtensionPath() {
return brackets.app.getApplicationSupportDirectory() + "/extensions/user";
if (brackets.app.getApplicationSupportDirectory) {
return brackets.app.getApplicationSupportDirectory() + "/extensions/user";
}

return null;
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/utils/ShellAPI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ define(function (require, exports, module) {
// which we have to return true (issue #3152).
return (eventName === Commands.FILE_CLOSE_WINDOW);
}
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;
Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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.


// This function should *only* be called as a top-level function. If the current
// stack depth is > 2, it is most likely because we are at a breakpoint.
Expand Down