Add JS linting ?

[jrfnl]

Inspired by https://github.com/squizlabs/PHP_CodeSniffer/issues/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:

• Either add a js lint tool to the Theme Check plugin and run it stand-alone and report the results separately.
• Or add either JSLint with Rhino, JSHint with Rhino and/or Javascript Lint to the Theme Check plugin and run the related PHPCS sniff(s) and let PHPCS handle the error reporting.

The path(s) to the tooling can be provided at runtime or added to the ruleset used by the Theme Check plugin.

<config name="jshint_path" value="/path/to/jshint.js"/>
<config name="jslint_path" value="/path/to/jslint.js"/>
<config name="jsl_path" value="/path/to/jsl"/>
<config name="rhino_path" value="/path/to/rhino"/>

Also see: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#setting-the-path-to-jslint

[grappler]

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. https://github.com/ternjs/acorn http://esprima.org/index.html

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.

[dingo-d]

I asked about ESLint config options back in November https://github.com/squizlabs/PHP_CodeSniffer/issues/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).

[grappler]

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?

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.

Also a tutorial on setting up correct linting options for ESLint would also be good (for working in Sublime or other IDE's).

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. 😄

[jrfnl]

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?

We have a few options. We could add the Human Made Coding standards as a dependency and use the sniff.

Just to be clear: we can only add the dependency in the Theme Check plugin, not in WPCS.

If it makes sense and there is no progress in PHPCS we could ask Ryan to make a PR to 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.

[dingo-d]

@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 :)

[grappler]

I used it for a little while, but it has to be manually configured to work properly so I kinda... delayed it xD

Even a blog post with a explanation and screenshots would be interesting.

According to the composer.json the code is GPL which is not compatible with MIT but if Ryan created the PR then there would be no issue with the licence.

[dingo-d]

I'm currently going through https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/ and creating a eslint.config.json file. When I'm done with it, should I post it here?

[jrfnl]

@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. Once it is, let's run some tests with it and decide whether the WP specific ES lint config would be best suited to be included with WPCS or in the Theme Check plugin or even elsewhere. I know there are some configs for JS tools included with WP itself - like jshint - which are often used in combination with build checks as well, so even that might be an option.

[dingo-d]

Ok, I've gone through the rules and I have made eslint.config.json file:

{
    "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:

• UpperCammelCase

  Cannot check for UpperCammelCase as this rule isn't available in eslint. We should maybe create a separate rule for that as a plugin. Reference issues: eslint#6626, eslint#8085.

• eqeqeq

  Currently set to smart. This needs to be tested. The smart seemed to best correspond to equality rule, so I set it as such.

Possible additions:

• no-empty-function
• new-parens
• no-unused-vars
• no-unexpected-multiline

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.

[jrfnl]

@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 Core or Extra.

From a TRCS perspective, a non-code style opinionated version would be needed which would just check for things like:

• parse errors
• probable bugs
• unused functions etc

[dingo-d]

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 no-empty-function will come in handy for this.

[dingo-d]

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 "extends": "eslint:recommended" so that not all of the rules that are enabled with it are enabled (some are stylistic, which we don't check).

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.

Hope this is ok @jrfnl @grappler

[jrfnl]

The ESlint sniff has been merged upstream and will be in the next PHPCS release (2.8.2/2.9.0).

[jrfnl]

@dingo-d Looks much better! All the same, it still feels opinionated. You include quite a few best practices, which - while I totally agree that theme devs should comply with -, are not something which is required as far as I know. At the same time, something like no-eval is not included which as far as I know is forbidden from a security perspective.

[dingo-d]

Oh, didn't know about the no-eval() rule. I've never used it or seen it being used in theme creation, which is why I didn't think that it's important. I can add it to the list.

I've left some rules in, which I thought would be a best practice, but they should throw a warning. Kinda like when phpcs reports warning on non escaped strings, and they end up being simple translations that can be ignored.

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).

[jrfnl]

I've left some rules in, which I thought would be a best practice, but they should throw a warning.

no-useless-concat, no-else-return just to name a few ?

I just had a quick look at the page you linked to, haven't reviewed in depth though.

[dingo-d]

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). It won't break the code, that's for sure, so I get why these would be best suitable for warnings.

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?

[justintadlock]

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.

[jrfnl]

@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

[justintadlock]

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.

[jrfnl]

@justintadlock Ok, thanks for the clarification. IMHO a sniff like that could possible be added to the WPCS Extra ruleset which is more focused on best practices, but has no place in the theme review CS as there is nothing wrong with the code itself.

[dingo-d]

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 curly, eqeqeq and such?

[jrfnl]

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. Anything else, only if it is explicitely mentioned in the theme review guidelines.

[dingo-d]

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.

[colorful-tones]

This post is super helpful and covers setting up Eslint for most major IDE: https://webdevstudios.com/2017/04/06/lint-code-like-boss/

[dingo-d]

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.

[colorful-tones]

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

[dingo-d]

You can check the above discussion, and supplied lint configurations and see if there is something you think should be added :)

[colorful-tones]

Also a tutorial on setting up correct linting options for ESLint would also be good (for working in Sublime or other IDE's).

Here you go: https://webdevstudios.com/2017/04/06/lint-code-like-boss/

You can check the above discussion, and supplied lint configurations and see if there is something you think should be added :)

I'll try to make some time today to do so. :)

[dingo-d]

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 https://github.com/WPTRT/WordPress-Coding-Standards/issues/120#issuecomment-285978768, and errors in https://github.com/WPTRT/WordPress-Coding-Standards/issues/120#issuecomment-290942033

[dingo-d]

I'm not sure we need this, as we'll try to cover the linting in the Theme Sniffer.

@timelsass Any thoughts?

[timelsass]

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. https://github.com/eslint/eslint/issues/8348 is a reference indicating that browser support is not a viable goal right now. Using the esprima method in https://github.com/WPTRT/theme-sniffer/pull/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.

[dingo-d]

The esprima approach sounds good to me too, I'm fine adding the .eslintrc for this, although the best way for doing (imo) this would be to separate this as a new repo (like what WPCS is doing in their eslint repo).