Introducing JSHint.

[valera-rozuvan]

Description

I want to propose the following guidelines to all future JavaScript code changes.

1.) All authors that write and submit JavaScript code (as a pull request, or a separate commit) should run JSHint against the code. The configuration file .jshintrc (to be used by all developers) will be placed in the edx-platfoirm root. When jshint runs, it first checks for .jshintrc file in the same directory as the source file, and then travels up the source tree, until it either finds a .jshintrc file or (if no .jshintrc file is found) uses built-in defaults. Our aim that all developers run JSHint with a standard configuration file, and that there are no hard-errors (see NOTE at the end) reported by JSHint.

2.) When in doubt about JSHint errors/warnings - use the latest JSHint from http://www.jshint.com/install/.

3.) All new JavaScript files should use the following template:

(function (globalVar1, globalVar2, ...) {
    'use strict';
    // Your code goes here. In this code you can use globalVar1 etc.
    // and not get an "used before defined" JSHint error. Also, being
    // explicit about what is global is a good way to go!
}).call(this, window.fullNameGlobalVar1, window.fullNameGlobalVar2, ...);

4.) All existing JavaScript files that are modified should be updated so that they pass against JSHint using the edx-platform .jshintrc file, and rewritten to use the wrapper anonymous function template from above.

NOTE: I believe that only JSHint warnings should be disregarded of the type "unused variable", when that unused variable is a function parameter that can't be removed. For example:

(function ($) {
    'use strict';

    var test = ['a', 'b', 'c'];

    $.each(test, function (index, value) {
        console.log(value);
    });
}).call(this, window.jQuery);

In this case JSHint will give a warning about index not being used. It is safe to disregard these kind of warning.

Special thanks to @jmclaus for providing his original .jshintrc file! It was modified a bit.

Motivation

Often, JSHint will catch silly errors right in the editor. You will see that you have coded something wrong without having to start up Studio or LMS and checking your changes in the browser. We all know the pain of waiting for the system to start up only to find that you have let a little JavaScript syntax error slip by.

Also, for a long time we are without a JavaScript standard and guidelines. I think with the introduction of JSHint, JavaScript code in edX will start to be formalized and standardized.

Reviewers

I want to hear the thoughts about this PR from all JavaScript developers at edX who are interested in improving the quality of JavaScript code at edX. Especially from:

• @singingwolfboy
• @cahrens
• @polesye
• @jmclaus

[jmclaus]

@valera-rozuvan Including .jshintrc at the root of edx-platform is a good idea.

Regarding 3. shouldn't we encourage the use of RequireJS modules instead:

//Calling define with module ID, dependency array, and factory function
define('myModule', ['dep1', 'dep2'], function (dep1, dep2) {
    //Define the module value by returning a value.
    return function () {};
});

And if we were to use the Module Pattern, I don't see the advantage of:

(function (globalVar1, globalVar2, ...) {
    ….
 }).call(this, window.fullNameGlobalVar1, window.fullNameGlobalVar2, …);

versus

(function (globalVar1, globalVar2, ...) {
    ….
 })(window.fullNameGlobalVar1, window.fullNameGlobalVar2, …);

[valera-rozuvan]

@jmclaus

• Regarding RequireJS.

Only Studio has RequireJS fully integrated. At this stage I don't think this is a good idea to enforce RequireJS for all JavaScript code.

• Using .call(this, ...) for the top level anonymous function.

If you use 'use strict'; then by default the top-level anonymous function doesn't get this set up as window. Before the strict mode was introduced, this was the default behavior - when this was undefined, it pointed to window. The approach I suggest will allow for code that used a top level this to refer to window to continue to work with the top-level anonymous function.

[jmclaus]

@valera-rozuvan Thanks for the explanations!

[cahrens]

Adding @andy-armstrong.

Personally, I don't feel the pain as much because I use an IDE that flags JavaScript coding issues (and can integrate with JSHint). I am concerned about adding a manual step to run JSHint outside of the build process.

[valera-rozuvan]

@cahrens This PR is mostly about introducing a .jshintrc file, and the process of JavaScript development that we should strive for. After it is in the system, we can then ask @jzoldak to setup an automatic job that will run checking PRs with JSHint against this .jshintrc file. We can then figure out criteria when to trigger an alert based on the output of JSHint.

[andy-armstrong]

Like @cahrens, I also use PyCharm's built-in JSLint integration, so I see most problems before I commit. I think it is a great idea to add some kind of JSHint/JSLint integration into the code quality checks that we do on Jenkins. In particular, it would be great if we could do a diff-cover style test that made sure that no new code has violations.

I'm not sure I like the idea of having to wrap all of our requireJS files with the extra boilerplate in #3. Ideally we'd be able to support both styles rather than require such a large rewrite.

Anyway, if this is just a best practice for the moment then I'm fine with checking this in and continuing the conversation. 👍

[valera-rozuvan]

@cahrens Are you thumbs up with this?

[valera-rozuvan]

@andy-armstrong I didn't think about RequireJS module files. Yes, I agree. We shouldn't add an extra wrapping for a define() or require() top level wrapper call.

[cahrens]

I am still unclear on what merging this PR means. I also do not want to wrap our CMS JavaScript with the boilerplate. I am very concerned about having to fix every issue in code that I touch (but was not originally written by me, so I may have incomplete understanding of it and could introduce bugs while trying to fix it).

[andy-armstrong]

@cahrens My thinking is that we should treat this like our other code quality tools where we set now as the baseline (or maybe sightly above to allow for a few new failures). All new code needs to add no new failures, and over time we can clean up the old code. @jzoldak, what's your thinking about this?

[valera-rozuvan]

@cahrens This .jshintrc file will be used by the JSHint (if available), and if it is configured (or rather it is using default configuration) to search for a .jshintrc file starting from the directory of the JavaScript file it is currently linting, and going up the tree. My intent was to define a common set of configuration options for JSHint to be used by all edX developers by default.

Even if your editor/IDE already lints for you, most likely it is a JSLint or JSHint program. If it is JSHint - great! Then it will find the .jshintrc file, and use the settings from there.

Regarding fixing issues in code that you touch. I agree that this task might seem daunting at first. However, I think that this doesn't have to be done to the point. The process of bringing the edX JavaScript code base to a higher standard doesn't have to happen right away. We just have to start little by little.

When @jmclaus comes to your office next week, ask him. He recently took on a similar challenge while cleaning up the JavaScript code base for his MIT projects. @jmclaus correct me if I am wrong...

[cahrens]

Ok, my concerns are assuaged-- let's try this and iterate. :+1

[valera-rozuvan]

@polesye Do you aprove?

[polesye]

Lets try. 👍

[jmclaus]

@valera-rozuvan I was using JSHint for a while with the MITx project but with default values. When I added 'strict' to every JS file, hell broke loose, a lot of errors had not been picked up. So I decided to enforce a stricter set of these (the .jshintrc you slightly modified). After some pain, the files passed JSHint and all is run in strict mode now. But I understand @cahrens concern about having to modify JS files that someone wrote a long time ago, might introduce bugs etc. I didn't have that problem with the MITx project since I coded most files.

[valera-rozuvan]

@jmclaus So that's a thumb up from you? = )

[jmclaus]

@valera-rozuvan 👍

[valera-rozuvan]

@singingwolfboy Are you OK with the .jshintrc I am introducing?

[singingwolfboy]

👍