Skip to content

Loading…

Issue #377 - Build: Upgraded to Grunt 0.4.0 #423

Closed
wants to merge 2 commits into from

4 participants

@JamesMGreene
jQuery Foundation member

Build: Upgraded to Grunt 0.4.0. Fixes #377.

@jzaefferer jzaefferer commented on an outdated diff
Gruntfile.js
((28 lines not shown))
+ },
+ gruntfile: ['Gruntfile.js'],
+ qunit: ['qunit/**/*.js'],
+ addons: {
+ options: {
+ jshintrc: 'addons/.jshintrc'
+ },
+ files: {
+ src: ['addons/**/*.js']
+ }
+ },
+ tests: {
+ options: {
+ jshintrc: 'test/.jshintrc'
+ },
+ files: {
@jzaefferer jQuery Foundation member

This should work fine as files: [...], doesn't need the nested stuff. I might confuse something, but please give it a try. If it works, apply everywhere.

@JamesMGreene jQuery Foundation member

I thought the same thing ~1 week ago when I tried just what you said but it doesn't work... gives a nasty warning message:

Warning: Cannot use 'in' operator to search for 'src' in addons/**/*.js
Use --force to continue.

What you are thinking of can currently only be done as the value of a target without any options, etc. e.g.

tests: ['test/**/*.js']

Talked to @cowboy about it on IRC last week to confirm. At least 1 other Grunt collaborator thought that would be a great enhancement, though I forgot who... maybe @tkellen or @sindresorhus?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff
test/test.js
@@ -470,6 +470,7 @@ test("propEqual", 5, function( assert ) {
});
test("raises", 9, function() {
+ /*jshint es5:true */
@jzaefferer jQuery Foundation member

Why is this one needed?

@Krinkle jQuery Foundation member
Krinkle added a note

I'll answer this one, it's because jshint < 1.0 doesn't know that future reserved words are safe in ES3 (throws). They fixed it, but unless we're on 1.0.0+, this is needed for now.

@JamesMGreene jQuery Foundation member

What @Krinkle said. Here's what I get when this line is removed:

Running "jshint:tests" (jshint) task
Linting test/test.js...ERROR
[L482:C5] Expected an identifier and instead saw 'throws' (a reserved word).
  throws(
[L488:C5] Expected an identifier and instead saw 'throws' (a reserved word).
  throws(
[L497:C5] Expected an identifier and instead saw 'throws' (a reserved word).
  throws(
[L505:C5] Expected an identifier and instead saw 'throws' (a reserved word).
  throws(
[L513:C5] Expected an identifier and instead saw 'throws' (a reserved word).
  throws(
[L521:C5] Expected an identifier and instead saw 'throws' (a reserved word).
  throws(
[L533:C5] Expected an identifier and instead saw 'throws' (a reserved word).
  throws(
[L545:C5] Expected an identifier and instead saw 'throws' (a reserved word).
    throws(

Warning: Task "jshint:tests" failed. Use --force to continue.
@JamesMGreene jQuery Foundation member

I am, however, fixes those two leading spaces to be a tab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff
addons/phantomjs/runner.js
@@ -17,7 +17,8 @@
(function() {
'use strict';
- var args = require('system').args;
+ var args = require('system').args,
@jzaefferer jQuery Foundation member

Undefined variable lists come first:

var url, page,
    args = require('system').args;
@jzaefferer jQuery Foundation member
@JamesMGreene jQuery Foundation member

D'oh, I knew that. Will fix shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff
Gruntfile.js
((6 lines not shown))
}
);
});
-grunt.registerTask('default', 'lint qunit');
+grunt.registerTask('default', ['jshint', 'qunit']);
+grunt.registerTask('test', ['jshint', 'qunit']);
@jzaefferer jQuery Foundation member

Why bother with a separate test task if it does the same as the default task?

@Krinkle jQuery Foundation member
Krinkle added a note

It leaves room to change the default. It's more a pattern than a conscious decision.

Though I would recommend avoiding duplication here. As you may know, aliases can be nested and the array format is optional if there is only 1 item:

grunt.registerTask( "test", ["jshint", "qunit"]);
grunt.registerTask( "default", "test" );

Oh, and something about consistency in quotation marks. Although the decision over jquery projects in general is up in the air (I prefer single quotes, too), at least for now it's double quotes still.

We probably can't put it in jshintrc just yet (a few other code paths violate it), but lets do work towards that for new code.

@JamesMGreene jQuery Foundation member

As @Krinkle said, it's more a pattern thing, just as the "package.json" file now contains an npm test hook that executes grunt test.

@Krinkle, what were you referring to here?

We probably can't put it in jshintrc just yet (a few other code paths violate it), but lets do work towards that for new code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Krinkle Krinkle commented on an outdated diff
Gruntfile.js
((14 lines not shown))
}
);
});
-grunt.registerTask('default', 'lint qunit');
+grunt.registerTask("default", ["test"]);
+grunt.registerTask("test", ["jshint", "qunit"]);
@Krinkle jQuery Foundation member
Krinkle added a note

Define test before referring to it. I know grunt evaluates this much later, but it's a good practice to define things before referring to them.

@JamesMGreene jQuery Foundation member

Sure, done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff
Gruntfile.js
((8 lines not shown))
grunt.initConfig({
- pkg: '<json:package.json>',
+ pkg: "<json:package.json>",

pkg: grunt.file.readJSON("package.json"),

@jzaefferer jQuery Foundation member

Or just drop the line, since we don't need to property anyway.

@JamesMGreene jQuery Foundation member

Line dropped locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff
package.json
((7 lines not shown))
"devDependencies": {
- "grunt": "0.3.x",
- "grunt-git-authors": "1.0.0",
+ "grunt-cli": "0.1.6",
+ "grunt": "0.4.0",
+ "grunt-contrib-jshint": "0.2.0",
+ "grunt-contrib-qunit": "0.2.0",
+ "grunt-git-authors": "1.1.0",

Use tilde versions: ~1.1.0

@JamesMGreene jQuery Foundation member

@jzaefferer @Krinkle Are you guys OK with using tilde versions or do we want to leave them as-is so we can be 100% that they will continue to work?

@sindresorhus I love semver as much as the next guy but I also know that it isn't always followed to the T.... ;)

@JamesMGreene jQuery Foundation member

P.S. I see both jQuery core and jQuery UI have chosen not to use tilde versions as well.

The reason they did that was because of RC releases of grunt and contrib doing breaking changes and that's why we temporarily recommended setting an absolute version. Though this should not happen anymore and semver best practices should IMHO be followed.

@JamesMGreene jQuery Foundation member

OK, we're onboard with trying it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sindresorhus sindresorhus commented on an outdated diff
package.json
((7 lines not shown))
"devDependencies": {
- "grunt": "0.3.x",
- "grunt-git-authors": "1.0.0",
+ "grunt-cli": "0.1.6",

Don't add grunt-cli as dep, rather add it to travis.yml: https://github.com/gruntjs/grunt-contrib-compass/blob/master/.travis.yml#L7

@Krinkle jQuery Foundation member
Krinkle added a note

Firstly, we don't use Travis (we have our own Jenkins because Travis doesn't yet support what we need - we'll switch to something it does support). Anyway, we want plain npm install && npm test to work without additional commands or global dependencies.

@jzaefferer jQuery Foundation member

I disagree with that. We can make grunt-cli a prerequisite just like we exepct node and npm to be installed already.

Should we start using Travis, we can still install it as needed.

@JamesMGreene jQuery Foundation member

Will remove the dep, per @sindresorhus and @jzaefferer.

Sorry, @Krinkle, you're overruled on this one. :broken_heart:

@Krinkle jQuery Foundation member
Krinkle added a note

No worries, @jzaefferer makes a good point. I'll rid of it in a few other places as well (or move it to travis.yml, as appropriate).

@JamesMGreene jQuery Foundation member

@Krinkle Easy to figure out, I'm sure, but here's the ".travis.yml" file that I use in all of my Grunt-based projects:

language: node_js
node_js:
  - 0.8
before_script:
  - npm install -g grunt-cli

e.g. JamesMGreene/node-flex/.travis.yml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jzaefferer jzaefferer commented on an outdated diff
Gruntfile.js
((14 lines not shown))
}
);
});
-grunt.registerTask('default', 'lint qunit');
+grunt.registerTask("test", ["jshint", "qunit"]);
+grunt.registerTask("default", ["test"]);
@jzaefferer jQuery Foundation member

What pattern are you referring to here? Sure, there's other grunt files that have a test task. But what's the point when the default is the same? If we ever need separate tasks, we can add that.

@JamesMGreene jQuery Foundation member

Fair enough, changed locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JamesMGreene
jQuery Foundation member

OK, we good now?

@JamesMGreene
jQuery Foundation member

BTW, thanks for chiming in @sindresorhus!

@jzaefferer
jQuery Foundation member

Yep, this is good. Just squash this into one commit when merging. Thanks.

@JamesMGreene
jQuery Foundation member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 48 additions and 46 deletions.
  1. 0 {qunit → }/.jshintrc
  2. +35 −40 grunt.js → Gruntfile.js
  3. +4 −3 addons/phantomjs/runner.js
  4. +8 −3 package.json
  5. +1 −0 test/test.js
View
0 qunit/.jshintrc → .jshintrc
File renamed without changes.
View
75 grunt.js → Gruntfile.js
@@ -1,60 +1,55 @@
-/*global config:true, task:true*/
+/*jshint node:true */
module.exports = function( grunt ) {
grunt.loadNpmTasks( "grunt-git-authors" );
+grunt.loadNpmTasks( "grunt-contrib-jshint" );
+grunt.loadNpmTasks( "grunt-contrib-qunit" );
grunt.initConfig({
- pkg: '<json:package.json>',
qunit: {
qunit: [
- 'test/index.html',
- 'test/async.html'
+ "test/index.html",
+ "test/async.html"
// TODO figure out why this fails on our Jenkins server (Linux)
- // 'test/logs.html'
+ //"test/logs.html"
],
addons: [
- 'addons/canvas/canvas.html',
- 'addons/close-enough/close-enough.html',
- 'addons/composite/composite.html'
+ "addons/canvas/canvas.html",
+ "addons/close-enough/close-enough.html",
+ "addons/composite/composite.html"
// TODO same as above
- // 'addons/step/step.html'
+ // "addons/step/step.html"
]
},
- lint: {
- qunit: 'qunit/qunit.js',
- addons: 'addons/**.js',
- tests: 'test/**.js',
- grunt: 'grunt.js'
- },
- // TODO remove this once grunt 0.4 is out, see jquery-ui for other details
- jshint: (function() {
- function parserc( path ) {
- var rc = grunt.file.readJSON( (path || "") + ".jshintrc" ),
- settings = {
- options: rc,
- globals: {}
- };
-
- (rc.predef || []).forEach(function( prop ) {
- settings.globals[ prop ] = true;
- });
- delete rc.predef;
-
- return settings;
+ jshint: {
+ options: {
+ jshintrc: ".jshintrc"
+ },
+ gruntfile: [ "Gruntfile.js" ],
+ qunit: [ "qunit/**/*.js" ],
+ addons: {
+ options: {
+ jshintrc: "addons/.jshintrc"
+ },
+ files: {
+ src: [ "addons/**/*.js" ]
+ }
+ },
+ tests: {
+ options: {
+ jshintrc: "test/.jshintrc"
+ },
+ files: {
+ src: [ "test/**/*.js" ]
+ }
}
-
- return {
- qunit: parserc( "qunit/" ),
- addons: parserc( "addons/" ),
- tests: parserc( "test/" )
- };
- })()
+ }
});
grunt.registerTask( "build-git", function( sha ) {
function processor( content ) {
var tagline = " - A JavaScript Unit Testing Framework";
- return content.replace( tagline, "-" + sha + " " + grunt.template.today('isoDate') + tagline );
+ return content.replace( tagline, "-" + sha + " " + grunt.template.today("isoDate") + tagline );
}
grunt.file.copy( "qunit/qunit.css", "dist/qunit-git.css", {
process: processor
@@ -84,7 +79,7 @@ grunt.registerTask( "testswarm", function( commit, configFile ) {
} )
.addjob(
{
- name: 'QUnit commit #<a href="https://github.com/jquery/qunit/commit/' + commit + '">' + commit.substr( 0, 10 ) + '</a>',
+ name: "QUnit commit #<a href='https://github.com/jquery/qunit/commit/" + commit + "'>" + commit.substr( 0, 10 ) + "</a>",
runs: runs,
browserSets: config.browserSets
}, function( err, passed ) {
@@ -96,6 +91,6 @@ grunt.registerTask( "testswarm", function( commit, configFile ) {
);
});
-grunt.registerTask('default', 'lint qunit');
+grunt.registerTask("default", ["jshint", "qunit"]);
};
View
7 addons/phantomjs/runner.js
@@ -17,7 +17,8 @@
(function() {
'use strict';
- var args = require('system').args;
+ var url, page,
+ args = require('system').args;
// arg[0]: scriptName, args[1...]: arguments
if (args.length !== 2) {
@@ -25,8 +26,8 @@
phantom.exit(1);
}
- var url = args[1],
- page = require('webpage').create();
+ url = args[1];
+ page = require('webpage').create();
// Route `console.log()` calls from within the Page context to the main Phantom context (i.e. current `this`)
page.onConsoleMessage = function(msg) {
View
11 package.json
@@ -29,9 +29,14 @@
"jquery"
],
"main": "qunit/qunit.js",
+ "scripts": {
+ "test": "grunt"
+ },
"devDependencies": {
- "grunt": "~0.3.17",
- "grunt-git-authors": "1.0.0",
- "testswarm": "1.0.1"
+ "grunt": "~0.4.0",
+ "grunt-contrib-jshint": "~0.2.0",
+ "grunt-contrib-qunit": "~0.2.0",
+ "grunt-git-authors": "~1.1.0",
+ "testswarm": "~1.0.1"
}
}
View
1 test/test.js
@@ -470,6 +470,7 @@ test("propEqual", 5, function( assert ) {
});
test("raises", 9, function() {
+ /*jshint es5:true */
function CustomError( message ) {
this.message = message;
}
Something went wrong with that request. Please try again.