Add JS linting ? #120
Comments
|
I think this would be good. I don't think I would go with JSLint or JSHint as they do not only check for syntax but for coding standards too. Additional tools that only do syntax checking. After doing a bit of research before it seems that ESLint is the latest and greatest for JS code sniffing. If I understand it correctly it requires node.js. Human Made has also created a PHPCS extension for it. https://github.com/humanmade/coding-standards/blob/master/HM/Sniffs/Debug/ESLintSniff.php Not completely relevant but a lot of the JS is from libraries. It would be interested to verify that there have been no change have been made to the libraries. |
|
I asked about ESLint config options back in November squizlabs/PHP_CodeSniffer#1210. So far it's added as a feature request, I don't see why they don't use rmccue's sniff for it (he even offered to make a PR). Can this be added in this fork of code sniffer, or should we be patient and wait for the official release from the squizilabs guys? Also a tutorial on setting up correct linting options for ESLint would also be good (for working in Sublime or other IDE's). |
We have a few options. We could add the Human Made Coding standards as a dependency and use the sniff. If it makes sense and there is no progress in PHPCS we could ask Ryan to make a PR to WPCS.
I have never used ESLint before but it sounds like you are offering to try it out and help the rest of us get up to speed. 😄 |
Just to be clear: we can only add the dependency in the Theme Check plugin, not in WPCS.
The best solution would be if @rmccue would be so kind as to pull the existing sniff against PHPCS. If that would be rejected or for some other reason not merged, it would be second best if he'd pull it against WPCS. We had quite a big discussion about licensing and pulling in sniffs from other standards in WPCS a while back and we have to be really careful about the legalities involved. |
|
@grappler I used it for a little while, but it has to be manually configured to work properly so I kinda... delayed it xD I'll be going through the WP coding standards in the coming weeks in a bit more detail so I'll see what I can do :) As for the coding standards by Ryan, I don't see any licence explicitly set on his sniffs, don't know if the licencing follows some kind of 'up the tree' rule (where all the code made by Human Made is made under GPL or MIT or similar), but we can always ask him :) |
Even a blog post with a explanation and screenshots would be interesting. According to the |
|
I'm currently going through https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/ and creating a |
|
@dingo-d That would be a great starting point. Ryan has opened a PR for the ESLint sniff upstream in PHCPS, just waiting for it to get merged. |
|
Ok, I've gone through the rules and I have made {
"root": true,
"globals": {
"_": false,
"$": false,
"Backbone": false,
"JSON": false,
"jQuery": false,
"wp": false
},
"env": {
"browser": true,
"node": true,
"jquery": true,
"es6": true
},
"rules": {
"array-bracket-spacing": [ "error", "always", { "singleValue": false, "objectsInArrays": true, "arraysInArrays": true } ],
"block-spacing": "error",
"brace-style": [ "error", "1tbs" ],
"camelcase": [ "error" ],
"comma-spacing": [ "error", { "before": false, "after": true } ],
"default-case": "error",
"eol-last": [ "error", "always" ],
"eqeqeq": [ "error", "smart" ],
"func-call-spacing": [ "error", "never" ],
"indent": [ "error", "tab", { "SwitchCase": true, "MemberExpression": true, "ArrayExpression": true, "ObjectExpression": true } ],
"key-spacing": [ "error", { "beforeColon": false, "afterColon": true, "singleLine": { "beforeColon": false, "afterColon": true }, "multiLine": { "beforeColon": false, "afterColon": true }, "align": { "beforeColon": false, "afterColon": true, "on": "colon" } } ],
"lines-around-comment": [ "error", { "beforeBlockComment": true, "beforeLineComment": true, "allowBlockStart": true } ],
"newline-per-chained-call": [ "error", { "ignoreChainWithDepth": 2 } ],
"no-array-constructor": "error",
"no-else-return": "error",
"no-extra-semi": "error",
"no-mixed-spaces-and-tabs": [ "error", "smart-tabs" ],
"no-new-object": "error",
"no-trailing-spaces": [ "error", { "skipBlankLines": false } ],
"no-undef": "error",
"no-unneeded-ternary": "error",
"object-curly-newline": [ "error", { "multiline": true } ],
"object-curly-spacing": [ "error", "always" ],
"object-property-newline": [ "error", { "allowMultiplePropertiesPerLine": true } ],
"one-var-declaration-per-line": [ "error", "initializations" ],
"operator-linebreak": [ "error", "after" ],
"quote-props": [ "error", "as-needed" ],
"quotes": [ "error", "single" ],
"semi": [ "error", "always", { "omitLastInOneLineBlock": true } ],
"semi-spacing": [ "error", { "before": false, "after": true } ],
"space-after-keywords": "error",
"space-before-function-paren": [ "error", "never" ],
"space-in-parens": [ "error", "always", { "exceptions": [ "()", "{}", "[]", "empty" ] } ],
"space-infix-ops": "error",
"space-unary-ops": [ "error", { "words": true, "nonwords": false, "overrides": { "!" : true } } ],
"spaced-comment": [ "error", "always" ],
"valid-typeof": "error",
"yoda": [ "error", "always", { "onlyEquality": true } ]
}
}I will test this on some of my js files once I get the Sublime properly configured to lint inline. That's going to be a good indicator. I have files that were linted using the JSHint, and they should be fine. There are some things that we need to cover. Possible issues:
Possible additions:We should go through some of the rules, and see which may be beneficial. Also I guess some sort of unit testing will be required, once the PR goes through on PHPCS. I don't know if this will be done on their side or on ours. |
|
@dingo-d If I look at this config, it appears opinionated about code style which is something which the theme review checks should not be. However, it could be acceptable for WPCS From a TRCS perspective, a non-code style opinionated version would be needed which would just check for things like:
|
|
True, I will see which lints are checking for non coding style issues and create a new config that should be suitable for TRCS. The lints such as |
|
Ok, a new config that would be suitable for TR is here: {
"root": true,
"globals": {
"_": false,
"$": false,
"Backbone": false,
"JSON": false,
"jQuery": false,
"wp": false
},
"env": {
"browser": true,
"node": true,
"jquery": true,
"es6": true
},
"rules": {
"block-scoped-var": "warning",
"curly": "error",
"default-case": "error",
"eqeqeq": [ "warning", "smart" ],
"init-declarations": [ "warning", "always", { "ignoreForLoopInit": true } ],
"no-alert": "warning",
"no-eval": "error",
"no-compare-neg-zero" : "error",
"no-cond-assign": "error",
"no-console": "error",
"no-constant-condition": [ "warning", { "checkLoops": false } ],
"no-debugger": "error",
"no-delete-var": "error",
"no-dupe-args": "error",
"no-dupe-keys": "error",
"no-duplicate-case": "error",
"no-else-return": "error",
"no-empty": "error",
"no-empty-function": "warning",
"no-eq-null": "watning",
"no-extra-boolean-cast": "warning",
"no-extra-semi": "error",
"no-fallthrough": "error",
"no-func-assign": "error",
"no-global-assign": "error",
"no-implicit-coercion": "warning",
"no-implicit-globals": "warning",
"no-inner-declarations": ["error", "var"],
"no-lonely-if": "error",
"no-loop-func": "warning",
"no-multi-spaces": "error",
"no-octal": "error",
"no-octal-escape": "error",
"no-obj-calls": "error",
"no-redeclare": "error",
"no-return-assign": "error",
"no-self-assign": "error",
"no-self-compare": "error",
"no-sparse-arrays": "error",
"no-undef": "error",
"no-undefined": "error",
"no-unexpected-multiline": "error",
"no-unreachable": "error",
"no-unsafe-negation": "warning",
"no-unused-vars": "error",
"no-use-before-define": "error",
"no-useless-concat": "error",
"no-useless-return": "error",
"radix": [ "error", "as-needed" ],
"use-isnan": "warning",
"valid-typeof": "error",
"vars-on-top": "warning",
"wrap-iife": [ "warning", "outside" ],
"yoda": [ "warning", "always", { "onlyEquality": true } ]
}
}Some issues are set as warnings, some as errors. The list of rules is here. I've removed almost all stylistic lint rules. I didn't use Do we allow Regex in JavaScript (reffering to no-control-regex, no-div-regex, no-empty-character-class, no-invalid-regexp and no-regex-spaces rules)? What if we get theme that is intended to be used with REST API? In that case people might use React or Vue.js to handle the views. In most cases people use a lot of ES6 syntax when creating apps with React or Vue.js. Currently there is only one theme like that iirc, so we could cover that in the future. There are some lint rules that are meant for such rules, but in my 3 years of creating themes I've never used them - most of the JS in themes handles simple DOM manipulation. |
|
The ESlint sniff has been merged upstream and will be in the next PHPCS release (2.8.2/2.9.0). |
|
@dingo-d Looks much better! All the same, it still feels opinionated. |
|
Oh, didn't know about the I've left some rules in, which I thought would be a best practice, but they should throw a warning. For others, I've placed those I personally encountered when developing a theme in (like variables that are not used, of functions that are not called anywhere). |
I just had a quick look at the page you linked to, haven't reviewed in depth though. |
|
Well, a review would be good. Sure those can be set as a warning, but some are set as error based on the fact that you really shouldn't have useless concatenation, or else statement if you are returning inside a block statement (at least I was thinking along those lines :D). I'll see which of these would possibly break the code and then leave those as errors, others should be left as a warning. Should I see if there are some useful plugins that could be used for checking if the code is valid, and include it, or just use the default eslint rules? |
|
Frankly, I wish we could throw an error for "else returns" in PHP too. That bugs me to no end and would do wonders in cleaning up some code in themes. Having it as a warning would be good though. |
|
@justintadlock Just wondering: What bugs you about that ? There is nothing essentially wrong with code like that. This is the ESLint rule detail - just to make sure we're talking about the same thing: http://eslint.org/docs/rules/no-else-return |
|
Calling it an "error" was a joke if that didn't come across. But, I'll bite. :) It bugs me when there's unnecessary code. We should always be promoting what's considered best practice in both the PHP and JS world. And, one of those best practices is getting rid of unnecessary code like else returns. While I wouldn't really go so far as to fail it as an error, I would definitely be strongly in favor of putting up a warning. |
|
@justintadlock Ok, thanks for the clarification. |
|
So basically the lint rules for theme review should be only errors and only checking for faulty code. Any style resembling rules should be left out - like |
|
Checking for (parse) errors and faulty code is definitely ok. Checking for unused code can probably be grouped with that as unused code shouldn't be shipped with a theme. |
|
I think that these rules could be considered as a 'base' that check against errors in code and unused vars and empty functions or conditionals (which is basically a garbage code). {
"root": true,
"globals": {
"_": false,
"$": false,
"Backbone": false,
"JSON": false,
"jQuery": false,
"wp": false
},
"env": {
"browser": true,
"node": true,
"jquery": true,
"es6": true
},
"rules": {
"curly": "error",
"default-case": "error",
"no-eval": "error",
"no-compare-neg-zero" : "error",
"no-cond-assign": "error",
"no-console": "error",
"no-debugger": "error",
"no-delete-var": "error",
"no-dupe-args": "error",
"no-dupe-keys": "error",
"no-duplicate-case": "error",
"no-empty": "error",
"no-empty-function": "error",
"no-extra-semi": "error",
"no-global-assign": "error",
"no-multi-spaces": "error",
"no-octal": "error",
"no-octal-escape": "error",
"no-obj-calls": "error",
"no-redeclare": "error",
"no-self-assign": "error",
"no-self-compare": "error",
"no-undef": "error",
"no-undefined": "error",
"no-unexpected-multiline": "error",
"no-unreachable": "error",
"no-unsafe-negation": "error",
"no-unused-vars": "error",
"no-use-before-define": "error",
"no-useless-return": "error",
"valid-typeof": "error",
}
}What would be good, if it can be done, to be able to extend this ruleset, kinda like we have multiple different standards (Core, Extra, Docs etc.). So that we can extend it to check for proper code syntax for instance, or a different broader rules, depending on the usage (theme review, core etc.). Also some sniffs could then have 'warnings' that could be ignored (or could be mentioned as a best practice). If anybody has time and will, I'd be glad if somebody could go through this list and see if there are some lint rules that should be removed, but I really trimmed anything that could be considered as a style rule. |
|
This post is super helpful and covers setting up Eslint for most major IDE: https://webdevstudios.com/2017/04/06/lint-code-like-boss/ |
|
Nice article, but I didn't see any configuration files for ES lint ;) Plus we want to make a standard so that we can include it in the WPCS sniffs. |
|
Here is the referenced link to WDS's wd_s (starter theme) ESlint confit file, which is in that article: https://github.com/WebDevStudios/wd_s/blob/master/.eslintrc.js |
|
You can check the above discussion, and supplied lint configurations and see if there is something you think should be added :) |
Here you go: https://webdevstudios.com/2017/04/06/lint-code-like-boss/
I'll try to make some time today to do so. :) |
|
Just keep in mind, here we're only checking for possible code errors, I've covered all the styling that is specified in the official WordPress Coding Standards in #120 (comment), and errors in #120 (comment) |
|
I'm not sure we need this, as we'll try to cover the linting in the Theme Sniffer. @timelsass Any thoughts? |
|
I'm not sure lol. @grappler is correct that right now eslint requires node.js - which would be a limitation from the theme sniffer side of things. I'd like to see eslint get loaded via browser, but I haven't been following progress on that very closely lately. eslint/eslint#8348 is a reference indicating that browser support is not a viable goal right now. Using the esprima method in WPTT/theme-sniffer#142 works well for catching the basic syntax errors and stays inline with WP core's implementation, so I think that's going to be the "as good as it gets" right now solution. We could start writing some checks based on that AST too, but that could get into going down a rabbit hole. I do think eslint browser support is close, and I remember seeing a couple of implementations that worked mostly, but don't think they supported loading custom rulesets. |
|
The esprima approach sounds good to me too, I'm fine adding the |
Inspired by squizlabs/PHP_CodeSniffer#1271, I'd like to suggest running a javascript lint on themes to prevent issues with syntax errors which often cause cross-plugin-theme problems.
IMHO there are two ways to go about this:
The path(s) to the tooling can be provided at runtime or added to the ruleset used by the Theme Check plugin.
Also see: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#setting-the-path-to-jslint
The text was updated successfully, but these errors were encountered: