Note: JSHint has been replaced with ESLint

See https://www.drupal.org/node/2264205
and https://www.drupal.org/node/1955232

Problem/Motivation

Core JS is inconsistent and contains basic mistakes that are easily avoidable once we make people aware of them.

Proposed resolution

In order to keep our JS code consistent we should rely on a dedicated tool able to spot common and well known issues. Two exists for JS code: JSLint and JSHint. Both have the same capabilities with JSHint being more flexible and a better fit for us.

JSHint is a tool to detect errors and potential problems in JavaScript code.

This issue is about deciding on which options the JS code should be checked with. It is going to be the basis on which our coding standards will live. JSHint can be used as a regular script, as a node js module and is integrated in a few IDEs. Using JSHint will make sure the JS shipped doesn't contain syntax errors or leaking variables, and will make sure the code can be minified properly.

This set of options has been agreed upon, it builds up from the configuration of the jQuery project adding a few Drupal-related rules:

.jshintrc

{
  "browser"   : true,
  "curly"     : true,
  "eqeqeq"    : true,
  "expr"      : true,
  "forin"     : true,
  "latedef"   : true,
  "newcap"    : true,
  "noarg"     : true,
  "trailing"  : true,
  "undef"     : true,
  "predef"    : [
    "Drupal",
    "drupalSettings",
    "jQuery",
    "_",
    "matchMedia",
    "Backbone",
    "VIE",
    "CKEDITOR"
  ]
}

.jshintignore

core/misc/backbone
core/misc/ckeditor
core/misc/create
core/misc/farbtastic
core/misc/html5.js
core/misc/jquery.ba-bbq.js
core/misc/jquery.form.js
core/misc/jquery.js
core/misc/jquery.once.js
core/misc/modernizr
core/misc/normalize
core/misc/ui
core/misc/underscore
core/misc/vie
core/modules/tour/js/jquery.joyride-2.0.3.js
core/vendor
sites/*/files

It will allow us to guard against various mistakes and enforce a consistent code.

Remaining tasks

  • find the right place to put the configuration in. Coder? only attached to the coding standards page?

User interface changes

None.

API changes

None.

Workflow changes

Once all the core JS files are fixed, each patch should be run — manually — through JSHint to check coding standards are respected and that no new mistake that can be checked for are introduced by the patch.

Original report by Rob Loach

Alongside #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core and #1415788: Javascript winter clean-up, we should run our JavaScript through JSHint. The command-line interface for it is available at: https://github.com/jshint/node-jshint .

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

Issue tags: +JavaScript clean-up

big +1

We need 3 people, 1 week of their time and a core committer involved to get patches in. Otherwise this will take forever and we'll have to reroll unrelated patches all the time until it's finished.

First we should agree on the settings for jshint, what do you propose?

Also for the actual progress tracking I'd like to keep it in the JS cleanup issue, there is already a table to track all the js files, can you add a column to it please?

droplet’s picture

Share my jshint config. I used it for druapl development a long time.

.jshintignore

core/misc/ui
core/misc/jquery.js
core/misc/jquery.ba-bbq.js
core/misc/jquery.form.js
core/misc/jquery.cookie.js
core/misc/html5.js
core/misc/farbtastic
core/misc/jquery.once.js
core/vendor
sites

.jshintrc

{
    // Settings
    "passfail"      : false,  // Stop on first error.
    "maxerr"        : 100,    // Maximum error before stopping.


    // Predefined globals whom JSHint will ignore.
    "browser"       : true,   // Standard browser globals e.g. `window`, `document`.

    "node"          : false,
    "rhino"         : false,
    "couch"         : false,
    "wsh"           : true,   // Windows Scripting Host.

    "jquery"        : true,
    "prototypejs"   : false,
    "mootools"      : false,
    "dojo"          : false,

    "predef"        : [  // Custom globals.
        //"exampleVar",
        //"anotherCoolGlobal",
        //"iLoveDouglas"
		"Drupal"
    ],


    // Development.
    "debug"         : false,  // Allow debugger statements e.g. browser breakpoints.
    "devel"         : false,   // Allow developments statements e.g. `console.log();`.


    // ECMAScript 5.
    "es5"           : true,   // Allow ECMAScript 5 syntax.
    "strict"        : true,  // Require `use strict` pragma  in every file.
    "globalstrict"  : false,  // Allow global "use strict" (also enables 'strict').


    // The Good Parts.
    "asi"           : false,  // Tolerate Automatic Semicolon Insertion (no semicolons).
    "laxbreak"      : false,   // Tolerate unsafe line breaks e.g. `return [\n] x` without semicolons.
    "bitwise"       : false,   // Prohibit bitwise operators (&, |, ^, etc.).
    "boss"          : false,  // Tolerate assignments inside if, for & while. Usually conditions & loops are for comparison, not assignments.
    "curly"         : true,   // Require {} for every new block or scope.
    "eqeqeq"        : true,   // Require triple equals i.e. `===`.
    "eqnull"        : false,  // Tolerate use of `== null`.
    "evil"          : false,  // Tolerate use of `eval`.
    "expr"          : false,  // Tolerate `ExpressionStatement` as Programs.
    "forin"         : false,  // Tolerate `for in` loops without `hasOwnPrototype`.
    "immed"         : false,   // Require immediate invocations to be wrapped in parens e.g. `( function(){}() );`
    "latedef"       : true,   // Prohipit variable use before definition.
    "loopfunc"      : false,  // Allow functions to be defined within loops.
    "noarg"         : true,   // Prohibit use of `arguments.caller` and `arguments.callee`.
    "regexp"        : true,   // Prohibit `.` and `[^...]` in regular expressions.
    "regexdash"     : false,  // Tolerate unescaped last dash i.e. `[-...]`.
    "scripturl"     : true,   // Tolerate script-targeted URLs.
    "shadow"        : false,  // Allows re-define variables later in code e.g. `var x=1; x=2;`.
    "supernew"      : false,  // Tolerate `new function () { ... };` and `new Object;`.
    "undef"         : true,   // Require all non-global variables be declared before they are used.


    // Personal styling preferences.
    "newcap"        : true,   // Require capitalization of all constructor functions e.g. `new F()`.
    "noempty"       : true,   // Prohibit use of empty blocks.
    "nonew"         : true,   // Prohibit use of constructors for side-effects.
    "nomen"         : false,   // Prohibit use of initial or trailing underbars in names.
    "onevar"        : false,  // Allow only one `var` statement per function.
    "plusplus"      : false,  // Prohibit use of `++` & `--`.
    "sub"           : false,  // Tolerate all forms of subscript notation besides dot notation e.g. `dict['key']` instead of `dict.key`.
    "trailing"      : true,   // Prohibit trailing whitespaces.
    "white"         : false,   // Check against strict whitespace and indentation rules.
    "indent"        : 2       // Specify indentation spacing
}
nod_’s picture

Title: [Meta] Clean up the JavaScript with JSHint » [Policy, no patch] Clean up the JavaScript with JSHint

Updated the table in the other issue, let's just deceide on the config here.

droplet one looks good :) I'll have a closer look this week-end.

nod_’s picture

Title: [Policy, no patch] Clean up the JavaScript with JSHint » [Policy, no patch] Decide on JSHint configuration for JavaScript clean-up

And actually, let's make it part of the JS coding standard once we agree.

RobLoach’s picture

Could we just commit those configurations to /core itself and have jshint pick it up?

nod_’s picture

I would definitely go for that. As long as it doesn't end up in the core/misc directory… #1663622: Change directory structure for JavaScript files :)

nod_’s picture

Title: [Policy, no patch] Decide on JSHint configuration for JavaScript clean-up » [Policy] Decide on JSHint configuration for JavaScript clean-up
droplet’s picture

Could we just commit those configurations to /core itself and have jshint pick it up?

jshint seeks the config from CWD and traverses up

nod_’s picture

Title: [Policy] Decide on JSHint configuration for JavaScript clean-up » [Policy] Decide on JSHint or JSLint and it's configuration

Umm so actually, why not jslint? FYI the config I usually use is: /*jslint browser: true, plusplus: true, vars: true, white: true, indent: 2 */

JSLint has op-out settings, while JSHint has op-in ones. I'd rather use JSLint unless you have a very compelling argument against it. It is generally more strict and I prefer that. We might need to add sloppy: true for contrib.

The only thing I'm seeing is the need to have vars declared at the top of the function with JSLint (can't be turned off) which shouldn't be an issue since functions shouldn't be huge ; that way var definitions should always be in view while reading a scope.

(ok i'll stop with the title changes :p)

droplet’s picture

I remember that's an article said "JSHINT is designed for stupid" and I'm the stupid one. so I use it. haha

- every jsLint can do, jsHint also can do. (!?)
- error message in jsHint is more friendly
- jsHint-nodejs is official supported.
- drop-in config.
- jQuery & UI using it.

nod_’s picture

The kind of details I like jslint for is that for example (function ($) {})(jQuery) will throw an error since it should be (function ($) {}(jQuery)). Since both works, we got that mixed in core JS files. And I think the blunt error messages are a feature :)

Both are more or less compatible but jslint is better at keeping code consistent which I, as a maintainer, appreciate.

I mean I'm not dead set on using jslint, if the feature set of jshint is better and relevant we should be using that, but for core I want to be able to enforce a variant when an situation like up there happens. And documenting in the coding standard handbook is not enough, who read the PHP one recently?

I know I used it before but we should avoid using the jQuery argument. jQuery and jshint: same guys involved. It's not very meaningful here.

droplet’s picture

@nod_,
can you give more real examples ?

Above example is a configurable option in jsHint. It won't break jsHint users heart :) But I do think this idea worse than #1483396: [policy, no patch] JavaScript coding standards for variable declarations

Just found one thing in Lint only:
"Unexpected 'else' after 'return'".......

Back to configs:
"dangling _", should we care about it ?

nod_’s picture

Title: [Policy] Decide on JSHint or JSLint and it's configuration » [Policy] Decide on JSHint configuration

All right read some more doc, let's go with jshint. But their comments are really confusing https://github.com/jshint/jshint/blob/master/jshint.js#L260, I have no idea what true or false is supposed to activate or desactivate what it says in the comments. The wording is really annoying.

dangling_, don't remember seeing that in core or contrib JS, we don't need to enforce the option.

then we can add/change from droplet conf:

immed: true

And that looks good to me.

I didn't get your point about the variable declaration, you're saying we should drop trying to make a rule for that?

attiks’s picture

There's one other problem, when using white: true, indent: 2, the following isn't allowed:

/*jshint white: true, indent:2 */
function test (col) {
  "use strict";
  var ret;
  switch (col) {
    case 1:
      ret = 0;
      break;
    case 2:
      ret = 1;
      break;
  }
  return ret;
}

Line 6, column 5: Expected 'case' to have an indentation at 3 instead at 5.
Line 7, column 7: Expected 'ret' to have an indentation at 5 instead at 7.
Line 8, column 7: Expected 'break' to have an indentation at 5 instead at 7.
Line 9, column 5: Expected 'case' to have an indentation at 3 instead at 5.
Line 10, column 7: Expected 'ret' to have an indentation at 5 instead at 7.
Line 11, column 7: Expected 'break' to have an indentation at 5 instead at 7.

FYI: Should be added to the next version: https://github.com/jshint/jshint-next

attiks’s picture

Forget to mention, I'm in big favor of JSHint

nod_’s picture

I don't mind dropping the whitespace option until it's fixed. This is not the kind of issues we run into often.

RobLoach’s picture

There are more docs available over at http://www.jshint.com/options/ .

droplet’s picture

dangling_, don't remember seeing that in core or contrib JS, we don't need to enforce the option.

core/modules/overlay/overlay-parent.js: line 888, col 36, Unexpected dangling '_' in '_recordTabindex'.
core/modules/overlay/overlay-parent.js: line 891, col 18, Unexpected dangling '_' in '_hasTabindex'.
core/modules/overlay/overlay-parent.js: line 891, col 65, Unexpected dangling '_' in '_hasTabindex'.
core/modules/overlay/overlay-parent.js: line 925, col 37, Unexpected dangling '_' in '_hasTabindex'.
core/modules/overlay/overlay-parent.js: line 926, col 38, Unexpected dangling '_' in '_restoreTabindex'.
core/modules/overlay/overlay-parent.js: line 927, col 18, Unexpected dangling '_' in '_hasTabindex'.
core/modules/overlay/overlay-parent.js: line 927, col 48, Unexpected dangling '_' in '_hasTabindex'.
core/modules/overlay/overlay-parent.js: line 935, col 16, Unexpected dangling '_' in '_recordTabindex'.
core/modules/overlay/overlay-parent.js: line 946, col 16, Unexpected dangling '_' in '_restoreTabindex'.

I suggest to allow it.

immed: true

I understand your concerns. Personally, I think we should not force to this style. (function ($) {}(jQuery))

@#14,
I don't get it. Both tools give me same result ?

nod_’s picture

1) If we go by consistency, we should just fix overlay-parent.js since this is the only place something like that is used in core and it doesn't add much meaning. the two functions are meant to be $().each callbacks, I wouldn't have guessed with the "_".

2) Yeah no proper way to decide on that. If I follow consistency like in 1) you're right. That said immed:true is the default for both jslint and jshint, it's a pretty good argument for switching to (function ($) {}(jQuery)) let's go with it like the rest of Js devs.

attiks’s picture

@droplet, they do, but JSHint is going to solve it, JSLint isn't AFAIK. I would be nice if we could use the white option. I just wanted to point out the problem with the white option, but like @nod said in #16 it isn't a blocker.

droplet’s picture

FileSize
4.39 KB

I converted both configs.

what I see is only 2 differences:

vars declared at the top of the function with JSLint (can't be turned off)

jsLint can't turn off this:

Stopping. (32% scanned).

(no full errors log)

jsLint config:

online version

/*jslint browser: true, vars: true, white: true, indent: 2, maxlen: 80 */
predefined vars:

jQuery Drupal $

jslint-nodejs

jslint --indent 2 --white --maxlen 80 --browser --vars --predef Drupal --predef jQuery --predef $

nod_’s picture

All right, just for clarity I removed all the default values that are the same in the rc file to see what has been overridden (turns out everything that is false is). We should commit the whole thing in case defaults changes at some point.

I have 2 more concerns:
wsh: true sounds like it's an omission, it should be false
I'm tempted to to put plusplus: true (to be clear: forbid the use of ++ and --) is there any opinion about this one?

I'm amazed at how angry reading the comments makes me. It is fascinating.

nod_’s picture

Status: Active » Needs review
FileSize
3.14 KB

oups :)

nod_’s picture

FileSize
3.13 KB

Just grouped the environments together to make it more obvious which features are turned off.

Quick question, does forin: false means unfiltered loops are allowed? We need to require filtered for loops.

nod_’s picture

Issue tags: +Coding standards

and tag

attiks’s picture

Quick question: maxlen: 80 is only for comments or also for code? I sometimes write longer code lines.

nod_’s picture

Don't know how jshint think it should be but for us that'd be only for comments, just like in PHP.

nod_’s picture

Issue tags: +Novice, +js-novice

we'll need documentation for how to set it up and check the JS code. adding a novice tag to it.

droplet’s picture

wsh === windows in Lint

forin: true, agreed

Quick question: maxlen: 80 is only for comments or also for code? I sometimes write longer code lines.

var i ;
	i++;
	
	if(i == 'ewfewfjewikfjwelkfjwlk jfweklfjwel jfweklfjwel jfweklfjweljfweklfjweljfweklfjwel kfjlekwjf klwejf lwejfklw ') { 
	// fjwkrljewkljwel jrewlkfj
		
	}

test result: both tools warning it.

nod_’s picture

FileSize
3.07 KB

I think we could survive with 80 char lines in core. There are relatively few lines going over. Also this will help avoid crazy function nesting we usually see around here.

If i understand correctly you're saying wsh: true is required to run jshint on windows, is that right? I thought it was to allow global variables defined by the wsh inside a script.

updated file for forin, left wsh alone for now.

droplet’s picture

Damien Tournoud’s picture

From #30: passfail is defined twice, tabs/spaces are inconsistent in the last three lines.

There are also some configuration parameters that are defined in jshint.js but not in our file (the first of which jquery: true, I think we should have all the configuration parameters there for reference.

"latedef"     : false

I think we actually want this to be true.

"devel"       : false, // if logging globals should be predefined (console, alert, etc.)

I think we actually want this to be true.

camelcase   : true, // if identifiers should be required in camel case

This is implied, but I guess we want it to be false? We use var variable_name for variables, not var variableName.

esnext      : true, // if es.next specific syntax should be allowed

This is implied, but likely we want it to be false? (it basically adds the const keyword).

white       : true, // if strict whitespace rules apply

Should we use this for extra picky whitespace validation?

Damien Tournoud’s picture

I also agree we want wsh: false.

droplet’s picture

nod_’s picture

FileSize
3.06 KB

latedef Relying on hoisting sounds dangerous. What is someone changes function toto() {} to var toto = function () {} for some reason? it'll break the JS. Should stay false.

devel I'd rather not have people writing console.log and/or alert in the JS #1606362: Use throw when an ajax error occurs and extend JavaScript's Error object. That said contrib is a jungle so maybe that's needed. Any other opinions on this?

camelcase, actually, we don't JavaScript coding standards. Should stay true.

esnext, agreed on the false.

white, see #14, we should hold off until it's fixed in JSHint.

droplet’s picture

I'm a bit lost :)

Summary:

JSHint Options reference:

After all above discuss, the config now is:

.jshintrc (nodejs)

{
"browser"     : true, // if the standard browser globals should be predefined
"curly"       : true, // if curly braces around all blocks should be required
"eqeqeq"      : true, // if === should be required
"es5"         : true, // if ES5 syntax should be allowed
"immed"       : true, // if immediate invocations must be wrapped in parens
"jquery"      : true, // if jQuery globals should be predefined
"newcap"      : true, // if constructor names must be capitalized
"noarg"       : true, // if arguments.caller and arguments.callee should be
"noempty"     : true, // if empty blocks should be disallowed
"nonew"       : true, // if using `new` for side-effects should be disallowed
"nomen"       : true, // if names should be checked
"regexp"      : true, // if the . should not be allowed in regexp literals
"undef"       : true, // if variables should be declared before used
"scripturl"   : true, // if script-targeted URLs should be tolerated
"strict"      : true, // require the "use strict"; pragma
"trailing"    : true, // if trailing whitespace rules apply
"forin"       : true, // if for in statements must filter
"plusplus"    : true, // if increment/decrement should not be allowed
"camelcase"   : true, // if identifiers should be required in camel case
"indent"      : 2,
"predef"      : ["Drupal"],
"maxerr"      : 1000,
"maxlen"      : 80
}

inline config (nodejs / online version) (totally same to .jshintrc):

/*jshint forin:true, noarg:true, noempty:true, eqeqeq:true, bitwise:true, strict:true, undef:true, curly:true, browser:true, jquery:true, es5:true, indent:2, maxerr:1000, plusplus:true, white:true, immed:true, newcap:true, nonew:true, nomen:true, regexp:true, scripturl:true, trailing:true, camelcase: true, maxlen:80 */
/*global Drupal */

Notice: Options are default to FALSE
(https://github.com/jshint/jshint/blob/master/jshint.js#L47)

a simple demonstration

http://www.jshint.com/reports/614017

nod_’s picture

Yeah well, thanks to the cryptic comments I have no idea what the hell true/false are supposed to do. You're saying I actually listed the wrong stuff? if so I agree with Damien much more than what the post in #35 look like.

I'm too tired for this, can't understand what's going on anymore. Links to jshint reports like in #34 will help.

droplet’s picture

@nod_,

I have no idea what the hell true/false are supposed to do. You're saying I actually listed the wrong stuff?

Yes, me too. I added an example to verify the settings. #36 is the latest version.

EDIT #36, added "camelcase"

droplet’s picture

FileSize
46.68 KB

Error log based on #36 configs

jshint -v
0.7.1
Bad option: 'camelcase'.

Only GIT JSHINT is supported

jhodgdon’s picture

This issue needs an issue summary before people interested in coding standards can give it a useful review. Please?

Damien Tournoud’s picture

latedef Relying on hoisting sounds dangerous. What is someone changes function toto() {} to var toto = function () {} for some reason? it'll break the JS. Should stay false.

It's one of the reversed option ("if the use before definition should not be tolerated"). We want it to be true.

droplet’s picture

gagarine’s picture

As a side note. We should check how we can integrate jsHint with http://www.php.net/manual/en/book.v8js.php

I create an issue on coder #230771: Provide basic javascript syntax and comment review but I think this should be a separate project (say "js_coder").

For my point of view jsHint is a first step. We can check a lot of other drupal specific mistake.

nod_’s picture

All right after some testing I'm happy with this config, which is pretty much what droplet posted to start with, same as #36 with latedef added. I'd like to get agreement on this since the cleanup should be fairly straightforward and, once we get a committer's attention, pretty quick.

{
"browser"     : true, // if the standard browser globals should be predefined
"curly"       : true, // if curly braces around all blocks should be required
"eqeqeq"      : true, // if === should be required
"es5"         : true, // if ES5 syntax should be allowed
"immed"       : true, // if immediate invocations must be wrapped in parens
"latedef"     : true,
"jquery"      : true, // if jQuery globals should be predefined
"newcap"      : true, // if constructor names must be capitalized
"noarg"       : true, // if arguments.caller and arguments.callee should be
"noempty"     : true, // if empty blocks should be disallowed
"nonew"       : true, // if using `new` for side-effects should be disallowed
"nomen"       : true, // if names should be checked
"regexp"      : true, // if the . should not be allowed in regexp literals
"undef"       : true, // if variables should be declared before used
"scripturl"   : true, // if script-targeted URLs should be tolerated
"strict"      : true, // require the "use strict"; pragma
"trailing"    : true, // if trailing whitespace rules apply
"forin"       : true, // if for in statements must filter
"plusplus"    : true, // if increment/decrement should not be allowed
//"camelcase"   : true,
"indent"      : 2,
"predef"      : ["Drupal"],
"maxerr"      : 1000,
"maxlen"      : 80
}

So what we're left with is camelcase we can't use right now and devel. So do we want to "allow" people to write alert() and console.log() in their scripts? I'm voting no, Damien is for yes, other opinions on this?

Someone is up for the issue summary?

nod_’s picture

Summary updated, does it makes things clearer jhodgdon? How can it be better?

droplet’s picture

I'm okay with the config :) and this is safe to use "camecase" now. It will give a warning to unsupported version:

core/modules/user/user.permissions.js: line 0, col 0, Bad option: 'camelcase'.

I think should not add "devel". It's good to tell everybody to remove debug function before release their scripts.

Also, JSHint supported inline config. we can use it for special cases

http://www.jshint.com/reports/622016

jhodgdon’s picture

Regarding the issue summary - please use the issue summary template. All of that information is very useful for anyone reviewing the issue.

jhodgdon’s picture

Issue summary: View changes

+issue summary

nod_’s picture

That's all I can do for the summary :) I'm not very inspired, if someone can make it better please go ahead.

Damien, do you have arguments to support the devel: true change?

@droplet, interesting to see the inline config change. I really wish we can avoid it but good to know it's available.

attiks’s picture

I would also like to see devel: false, and like @droplet said if it's really needed we can override it for 1 function, but I would prefer it at the top of the function, not inline switching.

I use /*global Drupal:false */ at the top of my file, meaning I can use Drupal, but I cannot overwrite it, so I think the predef should be
"predef" : ["Drupal":false],

nod_’s picture

Well if all goes well Drupal won't be a global for long, and you'll be able to do whatever you want with it. So I'm unsure we need to go this far. And what about drupal.js? it does define Drupal, what should we do for that?

I think we just need a comment from Damien and hopefully get this RTBC soon.

About where the file should live: I'm not sure we want that in the root directory. should that be in core/example.jshintrc or… core/misc/example.jshintrc? (Just to make sure, second one is a joke). I'm putting it as example.jshintrc to follow what we're doing with the .gitignore. Ideally we'd have .jshintrc and .jshintignore at the root but…

(edit) Or do we want that in core at all? would it be better in drush/coder or something like timplunkett and xjm told me on IRC?

gagarine’s picture

I'm voting for contrib before this becoming mature and see if we can make a module like coder to check JavaScript. I'm going to try this v8js when I get some time.

nod_’s picture

Any advice which module we should submit that to?

RobLoach’s picture

Damien Tournoud’s picture

I'm fine with the configuration as is. It's a good objective for core javascript :)

nod_’s picture

Title: [Policy] Decide on JSHint configuration » [Policy, no patch] Decide on JSHint configuration
Status: Needs review » Reviewed & tested by the community

Awesome, we agree on the config, let's find the right place to put that now :)

I'm thinking starting by adding it to the coding standards page and try to get it in coder or something. I'll ping jhodgdon for guidance if one of you guys know better feel free to take action.

Will set to fixed once we have the config where it needs to be.

webchick’s picture

I don't see sun in this issue, and he is likely to have an opinion on it. Please reach out.

webchick’s picture

Also, we need an issue summary here that reflects the current recommendation.

webchick’s picture

Issue summary: View changes

issue template

nod_’s picture

Issue summary: View changes

summary update

nod_’s picture

Updated summary, tried to make the comments more obvious about what each option does. Sun is pinged about this.

webchick’s picture

Awesome; thanks a lot, nod_! :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Also, please put a draft of what you propose for the coding standards page/section in the issue summary. To me (someone who knows very little of these tools), the issue summary tells me nothing about what is really being proposed. Will all core code need to be checked with JSHint (and how do you run it)? When? What does JSHint do -- fix the code or tell you about errors? Can it be automated (on the test bot)?

nod_’s picture

I can reply for a couple things already,

All core javascript code will need to be checked, at least before D8. It's either a command line tool that runs with nodejs, so node npm jshint to install and jshint . to check the code. The errors are outputted on the command line. But it's a regular javascript script: can be used in the browser as well, like droplet showed: http://www.jshint.com/reports/614017

JSHint does not fix the code, it tells you about errors.

It can be automated as long as the testbot can run nodejs and/or possibly access to the V8 PHP extension gagarine linked to. So that won't be straightforward for now.

droplet’s picture

Status: Needs work » Needs review

Important:
(@Sun) Step #1: I think at this step we just need to confirm "JSHint config files" is correct. (It won't break old standard and new standard are good for Drupal)

Less Important:
(@nod_, drupaler) Step #2: Starting apply it to all CORE development.
(@jhodgdon, doc team) Step #3: Write into Docs (how to use it, etc...)

ref:
how to use it:
http://www.jshint.com/platforms/

jhodgdon’s picture

Status: Needs review » Needs work

OK, that helps a bit... So this is what is needed for the issue summary:
- What is the proposed coding standard for JS (no errors? only some types of errors allowed? ???)
- How to install and run the tool (detailed instructions on how to install all of the needed stuff, and instructions on how to run it in the command line -- or links to web sites providing this information).
- Some output of the tool on our current core JS code, or some way to see what changes would be made between current and corrected code.

Without this information, it's difficult for reviewers of this proposed standard to tell:
- How much of a burden this would be on developers to use the tool
- How much of an adjustment this would be for developers to write compliant code
And that is what goes into decisions, in general, on adopting new coding standards.

Thanks!

nod_’s picture

Status: Needs work » Needs review

I thinks #3 is on us as well droplet :) jhodgdon is here to check on it, not write the thing.

nod_’s picture

Status: Needs review » Needs work

sorry about that :p

gagarine’s picture

I made a module where I was able to run jshint with the v8js extension. https://drupal.org/sandbox/gagarine/1674242 .

It's a prof of concept...

sun’s picture

Under the assumption that the summary contains the latest proposal, here's my feedback:

I disagree with these:

"plusplus"    : true, // ++/-- is not allowed, use += 1 -= 1

There's no reason for disallowing these operators. Personally, I'd actually prefer the reverse, but there doesn't seem to be an option for that. In any case, I don't see why we'd want to disallow this. The increment/decrement operators or very handy in many situations, and in quite some situations they enable to write less code. (Whether it makes sense to use them always requires human evaluation, but that applies to all code that you write.) As long as the language supports them, they should be allowed.

"regexp"      : true, // The '.'  is not allowed in regexp literals

I don't see why this should be disallowed. The docs on this option are sparse, so I don't really understand what the reasons for disallowing it are. In general though, coding standards should not have any say on logical stuff like this, unless there are hard technical reasons for doing so. I do not see such reasons in this case.


I think we should add these:

"boss" : true, // Suppress warnings on variable assignments where "typically" comparisons are expected.
"expr" : true, // Suppress warnings where "typically" expressions are expected.

Such "typical" rules always smell like false-positive monsters to me. That's what code reviews are for. So we should suppress those.

Potentially also:

"regexdash" : true, // Suppress warnings on unescaped dash (-) at the end of regular expressions.
"supernew" : true, // Suppress warnings on singleton constructs.

These two smell a bit arbitrary to me. I'd rather suppress them for now, and potentially reconsider later, if needed.

And also, but debatable:

"nonstandard" : true, // Enable non-standard but widely adopted globals such as escape and unescape.

As it looks like we're going to target modern browsers only with D8, I wonder whether enabling these would make sense.


Lastly, it would be great if we could sort the option names alphabetically ;) Took me some time to check whether we're defining an option or not... ;)

gagarine’s picture

plusplus Some arguments to avoid it https://www.youtube.com/watch?v=taaEzHI9xyY&t=50m42s (50:42 - but anyone with time can watch the full presentation...)

regexp http://www.regular-expressions.info/dot.html (generally is better to be specific about what you want to catch)

Both can be used in the right way but can have unwanted consequence. Of course if you "know what you are doing" it's all right but lot of peoples don't and it make your code harder to understand.

I'm more to add warning and remove them if really they pose problem. It's easier to spot a problem they actually do somethings...

droplet’s picture

"supernew" : true, // @sun: Suppress warnings on singleton constructs.

This is not warning on singleton constructs, example: http://www.jshint.com/reports/627007

I would like to see: supernew: false
reason: http://yuiblog.com/blog/2006/11/13/javascript-we-hardly-new-ya/


I would like to see: "expr" : false
https://github.com/jshint/jshint/blob/master/tests/unit/options.js#L325
example: http://www.jshint.com/reports/625012

mfer’s picture

Reading through some of the comments here it seems like we are having a discussion where we need to decide how jshint is being used. On one side it feels like we are talking about a JS validator (to make sure we ship valid JS) and on the other side it sounds like good JS habits (but defined by who?). While I believe we need to make sure every release ships with a full set of valid and working JavaScript (for example D7.0 had broken locale JS) I'd like to see JSHint help us write better quality JavaScript.

Now, how do we decide which practices are the ones to follow? Do we allow ++/-- or not? If you follow JavaScript the good parts (by Crockford) than you don't use this. Because this can easily turn into a color of the bike shed debate and there are a lot of personal preferences around this type of thing I fear this conversation could go on for some time.

So, I propose Drupal use the same settings as jQuery. The jQuery settings have been widely agreed upon by some smart JS people. Sure, not everyone agrees. That's just part of coding conventions like these. But, by choosing to go with what jQuery already does we can sidestep the preference debate within Drupal to see whose preferences win.

The jQuery JSHint settings can be found at https://github.com/jquery/jquery/blob/master/.jshintrc and https://github.com/jquery/jquery/blob/master/src/.jshintrc. The commit that last updated these can be seen at https://github.com/jquery/jquery/commit/96246332f723b0f83e465d0f9f580741....

droplet’s picture

This is adopt both jQuery & existing Drupal JS standard. jQuery has Smart JS People review their codes. It's better than tools.

jQuery dropping some rules for a reason:
http://docs.jquery.com/JQuery_Core_Style_Guidelines

For "personal preference" rules, following jQuery we will easier to reach a consensus. We can drop these 4 from Config #44:
plusplus
regexp
immed
sub

nod_’s picture

So making things shorter by following what jquery does and merging that with our standards we get:

{
"browser"     : true,
"curly"       : true,
"eqeqeq"      : true,
"forin"       : true, // Drupal-specific, already fixed core bugs
"latedef"     : true,
"maxerr"      : 1000,
"newcap"      : true, // Drupal Coding Standard
"noarg"       : true,
"predef"      : ["Drupal", "jQuery"], // Drupal-specific
"trailing"    : true,
"undef"       : true
}

And with that there is still 117 errors to fix in our files :)

for expr in core we have:
ajax.js:628 while (match = importMatch.exec(response.data)) { legit.
tabledrag.js:280 self.dragObject === null ? $(this).addClass('tabledrag-handle-hover') : null; should be a if.
tabledrag.js:282 self.dragObject === null ? $(this).removeClass('tabledrag-handle-hover') : null; should be a if.
color.js:177

focused && $(focused).unbind('keyup', farb.updateValue)
          .unbind('keyup', preview).unbind('keyup', resetScheme)
          .parent().removeClass('item-selected');

should be a if.

I'm pretty sure you agree that's a pretty ugly way to go about it. I'll personally activate it during my code reviews, I'm fine with it not being included in this config.

How that look now?

gagarine’s picture

Their is one difference between jQuery and Drupal: At jQuery they know how to write JS, we mostly don't.

More warnings will highlight more potential errors. This is not like unit test, false positive are acceptable.

nod_’s picture

Ok, we really need to agree on this in a timely manner. Fixing that will take time we're running out of.

Even with these "restricted" sets of options, we have more than enough things to fix and it'll catch most of the errors. Like sun said, reviews count for something. Even with pretty much no options, jshint would spot basic mistakes that make JS break as mfer showed in #71.

Let's concentrate on the things that needs fixing now and not potential errors. We don't have the luxury to discuss this to no end anymore.

nod_’s picture

Status: Reviewed & tested by the community » Needs review
{
"browser"     : true,
"curly"       : true,
"eqeqeq"      : true,
"expr"        : true,
"forin"       : true, // Drupal-specific, already fixed core bugs
"latedef"     : true,
"maxerr"      : 1000,
"newcap"      : true, // Drupal Coding Standard
"noarg"       : true,
"predef"      : ["Drupal", "jQuery"], // Drupal-specific
"trailing"    : true,
"undef"       : true
}

forgot expr. got confused with this true/false thing again.

nod_’s picture

Issue summary: View changes

Make comments more understandable

attiks’s picture

Status: Needs work » Reviewed & tested by the community

I agree with @nod_ we have to decide, if it turns out later that we need some changes (for whatever reason) we can still reconsider.

Personally I would like to see #44 keeping in mind the comments from #68, but to get this moving I can also live with #73 for the sake of moving this forward. If we decide to go with #73 we need to create a follow-up to make it more restrictive in a follow-up issue.

Marking this as RTBC so people will notice.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

All right updated the conf in the issue summary. I don't know if you noticed the edit attiks, added the expr option. The boss option is about 1 line in all files, not significant enough to hold this up. It can be added later without changing anything in the cleanup.

I've removed the maxerr setting. We don't want that to be anywhere near 1000 errors. In core we have under 200 of them across all files. The followup issues will deal with each file/module individually and they won't have that many errors. The default of 50 errors is enough.

I'll get the follow-up issues created, we still need to discuss how to get that in coder/somewhere and update the relevant doc pages to "fix" the issue.

attiks’s picture

Let's start with this, it would be nice if we had an extension for coder, maybe something based on https://drupal.org/sandbox/gagarine/1674242 so we eventually can automate it on the testbots or at least people can do it locally, otherwise we have to check each patch manually :/

RTBC for real now.

nod_’s picture

Tracking progress over at #1415788: Javascript winter clean-up. Most of it is already down.

As for automation It's integrated to my IDE. I realize that's not the case for everyone but it's very handy if you have access to it.

gagarine’s picture

Ok good.

We should add a chapter in the documentation than contain this configuration and how to configure various IDE.

I'm all to add that in coder, but coder is actually not made for that, is why I started this new module. I will try to find time to add a real administration page. The execution of JS in PHP works now in my sandbox but i want to give the option to use node.js, V8js or a external http service (somthing like https://github.com/johnbender/jshint-service ). If someone is interested, you are more than welcome to continue this discussion directly on https://drupal.org/sandbox/gagarine/1674242

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

This coding standards issue says it is "reviewed and tested by the community", but I cannot tell from the issue summary what exactly has been agreed upon, or what the proposed new coding standard is.

The issue summary needs to be updated to say what the proposed resolution is:
- What will developers need to do?
- What coding standards pages need to be updated, and what proposed text is going to go on them?
- What will the test bot need to do (if anything)?
etc.

langworthy’s picture

Instead of:

"predef"      : ["Drupal", "jQuery"],

don't we want:

"predef"      : ["Drupal"],
"jquery"      : true,
langworthy’s picture

Issue summary: View changes

update config

Jelle_S’s picture

Just updated the issue summary and added

"unused"    :true // Declared variables must be used

see #1745964: JSHint: Remove unused variable declarations

Jelle_S’s picture

Issue summary: View changes

Declared variables must be used

nod_’s picture

Issue summary: View changes

fix config

seutje’s picture

Status: Needs work » Needs review

@83: I think the reason we're not using
jquery: true
but instead set it as a predefined variable, is because otherwise it allows the use of the dollarsign global, which we don't.

also: updated summary to add some missing whitespace.

nod_’s picture

It'll probably be pushed back because of #82, good tasks for new guys :)

nod_’s picture

Issue summary: View changes

add missing whitespace

seutje’s picture

seutje’s picture

Issue summary: View changes
seutje’s picture

Updated issue summary again, as per #1754344: Rename Drupal global JS object to drupal

oxyc’s picture

Question in reference to http://drupal.org/node/675446#comment-6423818

Because jQuery ui (and everyone) are binding this in callbacks, jshint currently throws a "Possible strict violation" error. There's an option to disable this validthis, but I'm guessing this is not really a smart move as this very setting saved myself from polluting the global object.

This option suppresses warnings about possible strict violations when the code is running in strict mode and you use this in a non-constructor function. You should use this option—in a function scope only—when you are positive that your use of this is valid in the strict mode (for example, if you call your function using Function.call).

How should be solve issues like these?

nod_’s picture

Core javascript is clean! Thank you all for the awesome work on the review and patches!

Now this needs to get some doc around if anyone is up for it.

#89, I like sending custom data with the event either when you bind or trigger so that you don't have to rely on this, usually you can just use e.target since that'll be the same DOM element as this would have been.

nod_’s picture

Issue summary: View changes

Updated issue summary. Changed DRUPAL to drupal in jshint settings object.

nod_’s picture

Updated defined globals from the conf. We now have:

"predef" : ["Drupal", "drupalSettings", "jQuery", "_", "matchMedia"], // Drupal-specific

nod_’s picture

Issue summary: View changes

update globals

rballou’s picture

Issue summary: View changes

Changed trailling to trailing

nod_’s picture

Issue summary: View changes

Removed undef, too messy.

nod_’s picture

Title: [Policy, no patch] Decide on JSHint configuration » [Policy, patch] Decide on JSHint configuration
FileSize
1.44 KB

Updated the config. I removed the undef option, it would be too much work and it can actually help contrib to have the list of all available parameters sometime. Updated the list of Global variables.

Created a sub-page in the doc, it needs work: http://drupal.org/node/1955232

And just in case, I'm attaching a patch to commit that to core.

Wim Leers’s picture

+++ b/.jshintignoreundefined
@@ -0,0 +1,18 @@
+core/misc/ui
+core/misc/ckeditor
+core/misc/underscore
+core/misc/modernizr
+core/misc/vie
+core/misc/backbone
+core/misc/create
+core/misc/jquery.js

Why no alphabetical order? :)

nod_’s picture

I can't speel

klonos’s picture

:D

jibran’s picture

Sorted the list added core/misc/normalize and removed core/misc/jquery.cookie.js it is now under core/misc/ui/external/jquery.cookie.js

Wim Leers’s picture

Tested this; it's good to go.

The only reasons I'm not yet RTBC'ing:
1) are we certain that we don't want to JSHint the "sites" and "modules" directories? Those who don't want to abide by the rules, can just ignore the warnings? We want to make it easy to follow the rules, no?
2) if we really do want to ignore the "sites" and "modules" directories, then why shouldn't we also ignore the "themes" directory?

JohnAlbin’s picture

Status: Needs review » Needs work

Following up on Wim's comments…

The patch in #96 adds these directories to the ignore list for JSHint:

core/misc/backbone
core/misc/ckeditor
core/misc/create
core/misc/farbtastic
core/misc/html5.js
core/misc/jquery.ba-bbq.js
core/misc/jquery.form.js
core/misc/jquery.js
core/misc/jquery.once.js
core/misc/modernizr
core/misc/normalize
core/misc/ui
core/misc/underscore
core/misc/vie
core/modules/tour/js/jquery.joyride-2.0.3.js
core/vendor
sites
modules

Note: the patch does not add the libraries or themes directory for contrib.

So, the core/* directories in that list make sense, since it excludes the 3rd party JS included with Drupal core. But the sites, modules, and themes directories are where an actual Drupal 8 installation stores its custom and/or additional 3rd party JS. Some of those scripts should be tested with JSHint and some should probably be excluded. But its impossible for Drupal 8 core to make that determination.

If a Drupal 8 installation wants to run JSHint on its entire codebase, it is highly likely they will need to modify the .jshintignore file to exclude 3rd party JS, but not exclude any custom JS. Having core's .jshintignore file have sites and modules in it means that core will, by default, ignore any custom JS in those directories. I think it is a mistake to ignore those custom JavaScripts by default; that determination should be left to the site maintainer.

So we should remove sites and modules from that ignore list.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.41 KB

Removed sites and modules dir.

Status: Needs review » Needs work

The last submitted patch, core-jshint-config-1664940-99.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
242 bytes
1.41 KB

Re-uploading.

nod_’s picture

Agreed with #98, I had contrib modules that bugged me, didn't clean up my ignore file.

We should be ignoring sites/*/files folders though. We don't want aggregated files showing up in there.

jibran’s picture

Added sites/*/files

nod_’s picture

Awesome RTBC for me. But I still don't know what's the policy for dot files in core.

droplet’s picture

We couldn't predict how user organize their files dir. (eg. multiple site..etc). jshint config file helps CORE development. For modules maintainers, they could run jshint command against their own module dir.

droplet’s picture

Issue summary: View changes

add .jshintignore file

droplet’s picture

+++ b/.jshintrcundefined
@@ -0,0 +1,22 @@
+"expr"        : true, // Drupal-specific, needs to be evaluated individually during review
+"forin"       : true, // Drupal-specific, already fixed core bugs
...
+"newcap"      : true, // Drupal Coding Standard

Also, the comments aren't make sense to me.

and as I know, GRUNT won't parse jshintrc with comments. Can we remove it ?

** When it hits bugs, meaningful message will show on the console.

nod_’s picture

Comments are fair, that's not proper JSON I took them out. Updated issue summary.

As for #105, I'm not sure we want to make it harder for contrib or webdev to be using jshint. We can do the same just fine for core, after all it's in core/ now.

I'll let droplet RTBC this if everyone is ok.

nod_’s picture

Issue summary: View changes

update config

droplet’s picture

I never develop a module in PURE env, that means when I run JSHINT on root dir, it shows thousands errors from other modules to me. It will make harder to everyone.

So, if excluded sites folder, modules, themes..

jshint => shows CORE errors
jshint sites/modules/name => one module errors
jshint CWD/* => shows current folders / sub folders errors

both will take the drupal .jshintrc / .jshintignore files (there's a reason not to move these config to CORE dir)

:)

nod_’s picture

i dont quite get what you're saying. how do we go to RTBC from here? the jshintignore is an extra the real important one is the RC. happy to drop the ignore one from the patch if that gets us a rtbc quicker.

droplet’s picture

I think it should add back

sites
modules
themes

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Core won't have any errors in the end, meaning doing a jshint on the root will be useless if we exclude user folders. It's the point of the move from sites/all to the root. If anything we should be excluding core/ folder to follow the current pattern.

Convenience of core contributor < convenience of contrib < convenience of users.

droplet’s picture

Okay.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ae07e9b and pushed to 8.x. Thanks!

alexpott’s picture

Title: [Policy, patch] Decide on JSHint configuration » Change notice: [Policy, patch] Decide on JSHint configuration
Status: Fixed » Active

Think this needs a change notice.

Jelle_S’s picture

nod_’s picture

Status: Active » Fixed
nod_’s picture

Title: Change notice: [Policy, patch] Decide on JSHint configuration » [Policy, patch] Decide on JSHint configuration

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nod_’s picture

All of core JS now validates against our configuration.

Wim Leers’s picture

YAY! nod_++

Wim Leers’s picture

Issue summary: View changes

update config files

ahwebd’s picture

Issue summary: View changes
ahwebd’s picture

Issue summary: View changes