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 .
Comment | File | Size | Author |
---|---|---|---|
#107 | core-jshint-config-1664940-107.patch | 1019 bytes | nod_ |
#103 | core-jshint-config-1664940-103.patch | 1.43 KB | jibran |
#101 | core-jshint-config-1664940-101.patch | 1.41 KB | jibran |
#101 | interdiff.txt | 242 bytes | jibran |
#99 | core-jshint-config-1664940-99.patch | 1.41 KB | jibran |
Comments
Comment #1
nod_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?
Comment #2
droplet CreditAttribution: droplet commentedShare my jshint config. I used it for druapl development a long time.
.jshintignore
.jshintrc
Comment #3
nod_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.
Comment #4
nod_And actually, let's make it part of the JS coding standard once we agree.
Comment #5
RobLoachCould we just commit those configurations to
/core
itself and have jshint pick it up?Comment #6
nod_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 :)Comment #7
nod_Comment #8
droplet CreditAttribution: droplet commentedjshint seeks the config from CWD and traverses up
Comment #9
nod_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)
Comment #10
droplet CreditAttribution: droplet commentedI 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.
Comment #11
nod_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.
Comment #12
droplet CreditAttribution: droplet commented@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 ?
Comment #13
nod_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:
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?
Comment #14
attiks CreditAttribution: attiks commentedThere's one other problem, when using
white: true, indent: 2
, the following isn't allowed: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
Comment #15
attiks CreditAttribution: attiks commentedForget to mention, I'm in big favor of JSHint
Comment #16
nod_I don't mind dropping the whitespace option until it's fixed. This is not the kind of issues we run into often.
Comment #17
RobLoachThere are more docs available over at http://www.jshint.com/options/ .
Comment #18
droplet CreditAttribution: droplet commentedcore/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.
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 ?
Comment #19
nod_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.Comment #20
attiks CreditAttribution: attiks commented@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.
Comment #21
droplet CreditAttribution: droplet commentedI converted both configs.
what I see is only 2 differences:
jsLint can't turn off this:
(no full errors log)
jsLint config:
online version
/*jslint browser: true, vars: true, white: true, indent: 2, maxlen: 80 */
predefined vars:
jslint-nodejs
jslint --indent 2 --white --maxlen 80 --browser --vars --predef Drupal --predef jQuery --predef $
Comment #22
nod_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 falseI'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.
Comment #23
nod_oups :)
Comment #24
nod_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 filteredfor
loops.Comment #25
nod_and tag
Comment #26
attiks CreditAttribution: attiks commentedQuick question: maxlen: 80 is only for comments or also for code? I sometimes write longer code lines.
Comment #27
nod_Don't know how jshint think it should be but for us that'd be only for comments, just like in PHP.
Comment #28
nod_we'll need documentation for how to set it up and check the JS code. adding a novice tag to it.
Comment #29
droplet CreditAttribution: droplet commentedwsh === windows in Lint
forin: true
, agreedtest result: both tools warning it.
Comment #30
nod_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 thewsh
inside a script.updated file for forin, left wsh alone for now.
Comment #31
droplet CreditAttribution: droplet commentedwe can keep the 80 char rule same to PHP:
http://drupal.org/coding-standards/#linelength
jsHint "wsh" equal to jsLint "windows" options
http://www.jslint.com/lint.html#windows
https://github.com/jshint/jshint/blob/master/jshint.js#L778
https://github.com/douglascrockford/JSLint/blob/master/jslint.js#L897
It should be
"wsh" : false
Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedFrom #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 whichjquery: true
, I think we should have all the configuration parameters there for reference.I think we actually want this to be true.
I think we actually want this to be true.
This is implied, but I guess we want it to be false? We use
var variable_name
for variables, notvar variableName
.This is implied, but likely we want it to be false? (it basically adds the
const
keyword).Should we use this for extra picky whitespace validation?
Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedI also agree we want
wsh: false
.Comment #34
droplet CreditAttribution: droplet commentedadd an example:
http://www.jshint.com/reports/614016
Comment #35
nod_latedef
Relying on hoisting sounds dangerous. What is someone changesfunction toto() {}
tovar toto = function () {}
for some reason? it'll break the JS. Should stayfalse
.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 staytrue
.esnext
, agreed on thefalse
.white
, see #14, we should hold off until it's fixed in JSHint.Comment #36
droplet CreditAttribution: droplet commentedI'm a bit lost :)
Summary:
JSHint Options reference:
After all above discuss, the config now is:
.jshintrc (nodejs)
inline config (nodejs / online version) (totally same to .jshintrc):
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
Comment #37
nod_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.
Comment #38
droplet CreditAttribution: droplet commented@nod_,
Yes, me too. I added an example to verify the settings. #36 is the latest version.
EDIT #36, added "camelcase"
Comment #39
droplet CreditAttribution: droplet commentedError log based on #36 configs
Only GIT JSHINT is supported
Comment #40
jhodgdonThis issue needs an issue summary before people interested in coding standards can give it a useful review. Please?
Comment #41
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt's one of the reversed option ("if the use before definition should not be tolerated"). We want it to be true.
Comment #42
droplet CreditAttribution: droplet commented+1
latedef: true
http://www.jshint.com/reports/612023
Comment #43
gagarine CreditAttribution: gagarine commentedAs 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.
Comment #44
nod_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.So what we're left with is
camelcase
we can't use right now anddevel
. So do we want to "allow" people to writealert()
andconsole.log()
in their scripts? I'm voting no, Damien is for yes, other opinions on this?Someone is up for the issue summary?
Comment #45
nod_Summary updated, does it makes things clearer jhodgdon? How can it be better?
Comment #46
droplet CreditAttribution: droplet commentedI'm okay with the config :) and this is safe to use "camecase" now. It will give a warning to unsupported version:
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
Comment #48
jhodgdonRegarding the issue summary - please use the issue summary template. All of that information is very useful for anyone reviewing the issue.
Comment #48.0
jhodgdon+issue summary
Comment #49
nod_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.
Comment #50
attiks CreditAttribution: attiks commentedI 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],
Comment #51
nod_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 aboutdrupal.js
? it does defineDrupal
, 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 asexample.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?
Comment #52
gagarine CreditAttribution: gagarine commentedI'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.
Comment #53
nod_Any advice which module we should submit that to?
Comment #54
RobLoachhttp://drupal.org/project/v8js ?
Comment #55
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm fine with the configuration as is. It's a good objective for core javascript :)
Comment #56
nod_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.
Comment #57
webchickI don't see sun in this issue, and he is likely to have an opinion on it. Please reach out.
Comment #58
webchickAlso, we need an issue summary here that reflects the current recommendation.
Comment #58.0
webchickissue template
Comment #58.1
nod_summary update
Comment #59
nod_Updated summary, tried to make the comments more obvious about what each option does. Sun is pinged about this.
Comment #60
webchickAwesome; thanks a lot, nod_! :)
Comment #61
jhodgdonAlso, 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)?
Comment #62
nod_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 andjshint .
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/614017JSHint 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.
Comment #63
droplet CreditAttribution: droplet commentedImportant:
(@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/
Comment #64
jhodgdonOK, 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!
Comment #65
nod_I thinks #3 is on us as well droplet :) jhodgdon is here to check on it, not write the thing.
Comment #66
nod_sorry about that :p
Comment #67
gagarine CreditAttribution: gagarine commentedI 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...
Comment #68
sunUnder the assumption that the summary contains the latest proposal, here's my feedback:
I disagree with these:
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.
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:
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:
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:
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... ;)
Comment #69
gagarine CreditAttribution: gagarine commentedplusplus
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...
Comment #70
droplet CreditAttribution: droplet commentedThis 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
Comment #71
mfer CreditAttribution: mfer commentedReading 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....
Comment #72
droplet CreditAttribution: droplet commentedThis 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
Comment #73
nod_So making things shorter by following what jquery does and merging that with our standards we get:
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
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?
Comment #74
gagarine CreditAttribution: gagarine commentedTheir 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.
Comment #75
nod_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.
Comment #76
nod_forgot expr. got confused with this true/false thing again.
Comment #76.0
nod_Make comments more understandable
Comment #77
attiks CreditAttribution: attiks commentedI 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.
Comment #78
nod_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.
Comment #79
attiks CreditAttribution: attiks commentedLet'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.
Comment #80
nod_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.
Comment #81
gagarine CreditAttribution: gagarine commentedOk 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
Comment #82
jhodgdonThis 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.
Comment #83
langworthy CreditAttribution: langworthy commentedInstead of:
don't we want:
Comment #83.0
langworthy CreditAttribution: langworthy commentedupdate config
Comment #84
Jelle_SJust updated the issue summary and added
see #1745964: JSHint: Remove unused variable declarations
Comment #84.0
Jelle_SDeclared variables must be used
Comment #84.1
nod_fix config
Comment #85
seutje CreditAttribution: seutje commented@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.
Comment #86
nod_It'll probably be pushed back because of #82, good tasks for new guys :)
Comment #86.0
nod_add missing whitespace
Comment #87
seutje CreditAttribution: seutje commentedUpdated issue summary as per #1754344: Rename Drupal global JS object to drupal
Comment #87.0
seutje CreditAttribution: seutje commentedUpdated issue summary as per #1754344: Rename Drupal global JS object to drupal
Comment #88
seutje CreditAttribution: seutje commentedUpdated issue summary again, as per #1754344: Rename Drupal global JS object to drupal
Comment #89
oxyc CreditAttribution: oxyc commentedQuestion 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.How should be solve issues like these?
Comment #90
nod_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 usee.target
since that'll be the same DOM element asthis
would have been.Comment #90.0
nod_Updated issue summary. Changed DRUPAL to drupal in jshint settings object.
Comment #91
nod_Updated defined globals from the conf. We now have:
"predef" : ["Drupal", "drupalSettings", "jQuery", "_", "matchMedia"], // Drupal-specific
Comment #91.0
nod_update globals
Comment #91.1
rballou CreditAttribution: rballou commentedChanged trailling to trailing
Comment #91.2
nod_Removed undef, too messy.
Comment #92
nod_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.
Comment #93
Wim LeersWhy no alphabetical order? :)
Comment #94
nod_I can't speel
Comment #95
klonos:D
Comment #96
jibranSorted the list added
core/misc/normalize
and removedcore/misc/jquery.cookie.js
it is now undercore/misc/ui/external/jquery.cookie.js
Comment #97
Wim LeersTested 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?
Comment #98
JohnAlbinFollowing up on Wim's comments…
The patch in #96 adds these directories to the ignore list for JSHint:
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.
Comment #99
jibranRemoved
sites
andmodules
dir.Comment #101
jibranRe-uploading.
Comment #102
nod_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.Comment #103
jibranAdded
sites/*/files
Comment #104
nod_Awesome RTBC for me. But I still don't know what's the policy for dot files in core.
Comment #105
droplet CreditAttribution: droplet commentedWe 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.
Comment #105.0
droplet CreditAttribution: droplet commentedadd .jshintignore file
Comment #106
droplet CreditAttribution: droplet commentedAlso, 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.
Comment #107
nod_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.
Comment #107.0
nod_update config
Comment #108
droplet CreditAttribution: droplet commentedI 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)
:)
Comment #109
nod_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.
Comment #110
droplet CreditAttribution: droplet commentedI think it should add back
Comment #111
nod_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.
Comment #112
droplet CreditAttribution: droplet commentedOkay.
Comment #113
alexpottCommitted ae07e9b and pushed to 8.x. Thanks!
Comment #114
alexpottThink this needs a change notice.
Comment #115
Jelle_SFYI: https://drupal.org/node/1684800#comment-7281704 (#1684800: JSHint drupal.js)
Comment #116
nod_Change notice: https://drupal.org/node/1972428
Comment #117
nod_Comment #119
nod_All of core JS now validates against our configuration.
Comment #120
Wim LeersYAY! nod_++
Comment #120.0
Wim Leersupdate config files
Comment #121
ahwebd CreditAttribution: ahwebd commentedComment #122
ahwebd CreditAttribution: ahwebd commented