Set up SCSS and JS linting

[jeffersonrabb]

Changes proposed in this Pull Request:

Match wp-calypso linting, so synced files no longer raise warnings.

What I've done so far:

• Added @wordpress/eslint-plugin, stylelint, stylelint-config-wordpress.
• .eslintrc as described in https://github.com/WordPress/gutenberg/tree/4786c14803859a046b14e5eb324b1041b8fbfff3/packages/eslint-plugin
• Copied lint npm scripts from https://github.com/Automattic/jetpack/pull/11767/files
• Copied .stylelintrc from Calypso (https://github.com/Automattic/wp-calypso/blob/46a9eafeda1d2d74871b527b25765fd7a9f7ae1e/.stylelintrc)

npm run lint correctly reports linting issues but errors out.

[adekbadek]

Calypso linting issue

This PR started as a an attempt to match Calypso linting configuration. Because Calypso pulls this code into their repo, they also lint it - and get a bunch of warnings.

Calypso uses custom configs for both stylelint and eslint. Copying these would not be a good idea because 1) they might change, 2) they're specific to Calypso codebase, at least the eslint part. Other way of resolving the warnings issue are discussed.

SCSS and JS linting

Nevertheless, this project did not have SCSS and JS linting set up - and that's that this PR evolved to be. It introduces following changes:

1. set up SCSS linting, using stylelint, with config based on stylelint-config-wordpress
2. set up JS linting, using eslint, with config based on @wordpress/eslint-plugin
3. add npm scripts to fire JS and SCSS linting
4. autofix of existing JS and SCSS errors and warnings
5. manual fix of other existing JS and SCSS errors and warnings
6. set up pre-commit git hook to run linting on staged files (for JS, it runs via xwp/wp-dev-lib which was already here (just missed eslint config). I've added SCSS handling via lint-staged
7. added documentation section in README.md so that it's all clear

One important thing 🚨

After this lands on your machine, run ./vendor/bin/cghooks update to update that git hook.

[claudiulodro]

This is really nice. Thanks for getting it set up. There is one small issue I noticed that would be good to solve if possible:

• With this branch, if you edit a PHP or JS file and something doesn't pass linting, the error will be output but the commit will still go through:

## ESLint
src/blocks/homepage-articles/index.js: line 24, col 7, Error - A space is required after '{'. (object-curly-spacing)
src/blocks/homepage-articles/index.js: line 24, col 34, Error - A space is required before '}'. (object-curly-spacing)
No staged files match any of provided globs.
[detached HEAD af529b7] Test commit <<< The commit went through here.

• If you check out the master branch or newspack-plugin, edit a PHP file and something doesn't pass linting, the commit will abort:

## PHP_CodeSniffer
E 1 / 1 (100%)

Time: 231ms; Memory: 12MB

src/blocks/homepage-articles/view.php:20:10: error - Expected 1 spaces before closing bracket; 0 found (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
Claudius-MacBook-Pro:newspack-blocks claudiulodro$  <<< The commit was aborted.

Since the linter should prevent people from pushing "bad" code, it would be nice to maintain the behavior where it only lets you commit when the linter passes.

[jeffersonrabb]

To test this out I tried comparing the linting warnings from Calypso to the warnings in newspack-blocks. To get the Calypso warnings, I checked current Calypso master and ran npm run lint, then kept an eye on warnings for files in apps/full-site-editing/full-site-editing-plugin/blog-posts-block/newspack-homepage-articles

It looks like there might be some rules that are in direct conflict. For example in .scss files in newspack-blocks I'm seeing a lot of Unexpected whitespace after "(" warnings, while in Calypso I see Expected single space after "(". I'm wondering if some of these rules need to be reconciled before we begin linting.

[adekbadek]

@claudiulodro That's fixed now with a33fa5e

@jeffersonrabb I've taken another look at it and updated the configs, trying to limit the warnings thrown on wp-calypso:

JS (eslint)

Linting errors (in wp-calypso) before: 354, after: 80

• no-undef for XMLHttpRequest and jsdoc/no-undefined-types for HTMLElement are triggered because wp-calypso's eslint config is not set to browser environment (in which these are defined). I've submitted a PR to wp-calypso repo to change that (Update eslint config to use browser environment wp-calypso#38634)
• ~80% of the remaining errors are triggered by wpcalypso/jsx-classname-namespace rule. It requires CSS class names to include file name in order to namespace them and avoid possible conflicts. This seems like a nice idea to me, WDYT? edit: as we discussed internally, this would lengthen the class names, which is undesirable considering AMP use

CSS (stylelint)

Linting errors (in wp-calypso) before: 53, after: 0

[claudiulodro]

I believe there are a couple tiny tweaks left, but this is almost there!

[claudiulodro]

I could be wrong, but I don't think this usage is correct. If you run npm run lint:scss the stylelint options are output, nothing is linted, and npm throws an error:

Claudius-MacBook-Pro:newspack-blocks claudiulodro$ npm run lint:scss

> [email protected] lint:scss /Users/claudiulodro/Documents/Projects/VVV/www/newspack/public_html/wp-content/plugins/newspack-blocks
> stylelint --syntax scss


  A mighty, modern CSS linter.

   Usage: stylelint [input] [options]

   Input: Files(s), glob(s), or nothing to use stdin.

     If an input argument is wrapped in quotation marks, it will be passed to
  globby for cross-platform glob support. node_modules are always ignored.
  You can also pass no input and use stdin, instead.

   Options:

     --config

       Path to a specific configuration file (JSON, YAML, or CommonJS), or the
       name of a module in node_modules that points to one. If no --config
       argument is provided, stylelint will search for configuration files in
       the following places, in this order:
         - a stylelint property in package.json
    {...ETC MORE OPTION OUTPUT HERE...}
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! [email protected] lint:scss: `stylelint --syntax scss`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the [email protected] lint:scss script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/claudiulodro/.npm/_logs/2020-01-08T20_28_04_855Z-debug.log

From the documentation that is conveniently output when this script is run :) :

   Usage: stylelint [input] [options]

   Input: Files(s), glob(s), or nothing to use stdin.

It looks like it's looking for the files in stdin; it should have something for [input]?

[adekbadek]

SCSS: that's on me, the npm script names were not descriptive enough. Fixed in 8473b98

Other problem is JS. This project already uses xwp/wp-dev-lib to do PHP & JS linting, but xwp/wp-dev-lib does not recognise .eslintrc.js (it assumes .eslintrc is the only way to configure Eslint). I've opened a PR there to change that (xwp/wp-dev-lib#313) - when that gets merged released then we're good.

If not, we can always run JS linting using lint-staged. But then if xwp/wp-dev-lib recognises .eslintrc.js someday, we'd be running JS linting twice.

[claudiulodro]

The latest version work great! Thanks for making the revisions and taking the initiative to contribute a PR back to wp-dev-lib. 👍

[matticbot]

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀