Add jshint to build process for javascript style issues

[ilinkuo]

Enhancement:

I'm seeing code like

if (F2.Apps[appConfig.appId] !== undefined)

in places. This kind of comparison is not recommended. For why, see http://stackoverflow.com/questions/776950/javascript-undefined-undefined or http://stackoverflow.com/questions/3390396/how-to-check-for-undefined-in-javascript.

In general, I recommend adding some sort of Javascript linting to the Travis-CI build process. I'd recommend JSHint over JSLint as you have more granular control over the style rules.

[brianbaker]

Few questions to anyone watching:

• What config options should we use?
• Should jshint be integrated into build -j such that it would run jshint and throw out warnings?
• Should a jshint build step be added into Travis (with the 'passfail = true' option)?
• It seems like the we would just run it against f2.no-third-party.js, so we don't get any warnings from the third party libs and can just focus on the code we write?

Depending on the config options chosen, there will likely be some issues to take care of to get rid of the warnings. I'm going to look to handle those issues as apart of this ticket unless anyone objects (perhaps create separate issues for each type of warning?).

Lastly, I'm not seeing a config option that would catch something like if (F2.Apps[appConfig.appId] !== undefined), though I could be overlooking it. If there isn't a config option for something like that, that particular type of comparison won't be found and solved by adding jshint.

[brianbaker]

For reference, here's the config I've been looking at locally:

{
    "browser" : true,
    "camelcase" : true,
    "curly" : true,
    "evil" : true,
    "globals" : { "easyXDM": true, "EventEmitter2": true, "jQuery": true },
    "latedef" : true,
    "noarg" : true,
    "quotmark" : "single",
    "strict" : true,
    "undef" : true,
    "unused" : true
}

[ilinkuo]

What config options should we use?

I would start by turning off everything you can turn off. JSHint will still catch a lot. Then turn on rules one by one as your team becomes comfortable with the rule set. The problem with the parent project, JSLint, was that it was very opinionated and had a lot of controversial rules even in its base rule set. JSHint's base rule set is pretty bland.

Should jshint be integrated into build -j such that it would run jshint and throw out warnings?

You'll have to deal with a large number of issues identified by JSHint on first run. I'd get rid of all of them -- either by correcting them, or by instructing JSHint that the practice is intentional. This gets the count down to zero. I would then integrate it into the master build. But then I'm undecided as to whether to have the build flag issues as errors which fail the build, as ignorable warnings, or to set a threshold for number of warnings before failing. My preference might also depend on whether the build was a CI build triggered by a checkin, or whether it was a nightly build.

Should a jshint build step be added into Travis (with the 'passfail = true' option)?
It seems like the we would just run it against f2.no-third-party.js, so we don't get any warnings from the third party libs and can just focus on the code we write?

Yes, I'd only run it against code which I control.

 Lastly, I'm not seeing a config option that would catch something like if (F2.Apps[appConfig.appId] !== undefined), though I could be overlooking it. If there isn't a config option for something like that, that particular type of comparison won't be found and solved by adding jshint.

You're right. JSHint doesn't pick up that one, AFAIK.

[brianbaker]

The build -j option will now run jshint on all of the source files and will not build/minify f2.js if there are any errors. Right now jshint is just checking the files that are in ./sdk/src but we can/should eventually expand its use to ./tests and ./examples. I'm also still investigating adding a pre-commit hook, but that might be somewhat difficult to get correct cross-platform (mac, windows, linux).

[brianbaker]

There are a few of these out there, but I'm using SublimeLinter to highlight the JS Hint issues in Sublime.