Add a GHA to check our JavaScript on pull requests (decide on JSLint / JSHint / ESLint)

[klonos]

This is a follow-up to #5578, where it was proposed to remove the JSHint files from our codebase, since we aren't using them. The goal here is to add a GHA that checks our JS code, same as we have one checking our PHP code.

Available options:

• JSLint: the "original" JS linter.
  • Hardcore devs seem to think that this is "what you should use if you want proper code".
  • It was forked to JSHint because it was too strict/rigid, opinionated, and with a fixed set of rules and configurations (people seem to think that it is complaining too much w/o an option to silence things that "are OK").
• JSHint:
  • It was created as a more configurable version of JSLint (of which it is a fork).
  • Supports a configuration file, making it easier to use in larger projects.
  • Has support for many libraries out of the box (like jQuery).
  • Difficult to know which rule is causing an error.
  • Has two types of option: enforcing and relaxing (which can be used to make JSHint stricter, or to suppress its warnings). This can make configuration slightly confusing.
  • No custom rule support.
  • This is what we seem to have inherited from Drupal (although in D8+ they have since moved to ESLint). This is also wort noting:

    JSHint is limiting its scope to issues of code correctness. If you would like to enforce rules relating to code style, check out the JSCS project.

  • JSCS:
    • It is a code style checker. It only catches issues related to code formatting - not potential bugs or errors.
    • The https://github.com/jscs-dev/node-jscs repo has been archived in Aug 2018, with the following note:

      JSCS has merged with ESLint!

    • It doesn’t do anything unless you give it a configuration file or tell it to use a preset. You can download configurations from their website, and it has a number of presets, such as the jQuery coding style preset.
• ESLint: the new(est) kid on the block. This is what Drupal 8+ uses.
  • Very flexible/extensible/configurable people seem to think (allows custom rules to fine-tune it to the specifics of a project):

    It is completely pluggable, every single rule is a plugin and you can add more at runtime.

  • It allows wrapping your code with eslint-disable to ignore things (similar to // cspell:disable/// cspell:enable we are using to ignore words in CSPell).
  • Newer versions of Drupal are using this, so perhaps their existing configuration is something that we can leverage in Backdrop(??).
  • https://www.drupal.org/node/2264205

    ESLint is designed to be pluggable unlike JSHint and JSLint. And will ship with stylistic rules, same rules that are dropped from JSHint. http://eslint.org/docs/rules

  • https://www.drupal.org/docs/develop/standards/javascript-coding-standards/eslint-settings

    As of Drupal 8, we use ESLint to make sure our JavaScript code is consistent and free from syntax error and leaking variables and that it can be properly minified.

  • Our Drupal brethren have progressively made it stricter and stricter, in order to start with some smaller changes, as they moved towards full linting.

I saw this in one of the sources, but wasn't able to confirm it:

ESLint is open source and licensed under the MIT License, which allows for more freedom in using and modifying the tool. JSLint, however, has a more restrictive license that places certain limitations on its use and distribution.

Another issue that would be a nice-to-have with our JS linting is to be able to detect deprecated jQuery functions and provide suggestions for these (see #6248). Here's what's available:

• https://github.com/dgraham/eslint-plugin-jquery
• https://github.com/wikimedia/eslint-plugin-no-jquery
  • a fork of eslint-plugin-jquery
  • The config plugin:no-jquery/deprecated includes all known deprecated and removed code, and is updated as new releases of jQuery come out. You can instead use configs targeting specific versions of jQuery if you know the environment in which your code will operate. There is a config for all minor versions from 1.0 to 3.6 (deprecated-1.0, ..., deprecated-3.7). Deprecation configs are cumulative, so they include all the rules for jQuery versions below them.

---

Sources

https://www.jslint.com https://jshint.com https://eslint.org

http://badassjs.com/post/3364925033/jshint-an-community-driven-fork-of-jslint https://web.archive.org/web/20130819215629/http://anton.kovalyov.net/2011/02/20/why-i-forked-jslint-to-jshint

D8 change record: Use ESLint to validate all Drupal JavaScript

https://stackoverflow.com/questions/6803305/should-i-use-jslint-or-jshint-javascript-validation https://www.sitepoint.com/comparison-javascript-linting-tools https://www.testim.io/blog/eslint-vs-jshint https://stackshare.io/stackups/eslint-vs-jslint https://medium.com/@sheldonled/from-jshint-to-eslint-8a0a135fa2bf https://www.quora.com/Which-is-better-JSHint-or-ESLint

[klonos]

I've assigned this issue to me for now, because I wanna try the JSHint GHA and configuration that @indigoxela has provided for https://github.com/backdrop-contrib/tinymce in this Zulip thread: https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/JavaScript.20linting.20tool (thank you 🙏🏼 )

@indigoxela:

After some experiments, I'm now happy with JSHint.

• No opinionated code style weirdness (doesn't actually care much about code style)
• Spots actual problems in code
• Super easy to integrate in a GitHub Action workflow
• Disadvantage: needs npm to install, but... :shrug:

Here's a workflow that integrates it: https://github.com/backdrop-contrib/tinymce/blob/1.x-1.x/.github/workflows/code-checks.yml#L35

To get nice inline annotations on Github for found errors, I had to create a custom report style, but that was easy:

https://github.com/backdrop-contrib/tinymce/blob/1.x-1.x/.github/misc/reporter.js

So if anyone else is interested in such a GHA, just go for it. :wink:

@herbdool:

Maybe for core?

@indigoxela:

Maybe for core?

Maybe... but we'd have to settle on a ruleset, which might take some time. https://github.com/backdrop-contrib/tinymce/blob/1.x-1.x/.jshintrc

And then we'd have to exclude all the third-party stuff... and finally fix all violations in our js code. Quite some effort...

Oh, almost forgot, you'll also need the options documentation to fiddle your own jshintrc:

https://jshint.com/docs/options

[klonos]

Here's a rough, PoC PR that runs ESLint on PRs: https://github.com/backdrop/backdrop/pull/4655

• It uses https://github.com/marketplace/actions/eslint-changed-files to only check changed files instead of the entire codebase. Ideally though we want something that will only check changed lines of code instead. See last point in this list.
• The .github/workflows/.eslintrc.json config file that I've used is the exact same as the one that ships with latest Drupal 11.x. Not sure if we want/need the same kind of checks, so that will need some clean-up and tweaking, and decide which checks we need for Backdrop.
• I couldn't get it to use the .github/workflows/package.json file, as it expects to find that file in the root of the repo. For now, I've found a workaround, which is to include things that would otherwise go into the devDependencies section of the package.json file by installing them directly in the GHA via run: npm install eslint eslint-config-airbnb-base.
• For testing purposes, and to confirm that this implementation works, I randomly picked core/misc/dialog.js in our codebase, and added a dummy comment line to it. This will need to be reverted - no linting is to happen as part of this PR.
• Some of the ESLint warnings in the PR are actually coming from https://prettier.io, which is what is used in Drupal. I don't mind it, but others might disagree. Can that be tweaked/customized so that it fits our coding standards? (in case they differ from those for Drupal).

• Next step is to tweak the implementation to be using something like https://github.com/grvcoelho/lint-diff. Here's why:

  ESLint is a great tool to enforce code style in your code, but it has some limitations: it can only lint entire files. When working with legacy code, we often have to make changes to very large files (which would be too troublesome to fix all lint errors) and thus it would be good to lint only the lines changed and not the entire file.

  lint-diff receives a commit range and uses ESLint to lint the changed files and filter only the errors introduced in the commit range (and nothing more).

  If we can get that to work, then we can commit this and start using it. If we can't get this to work, we'll nee to come up with a plan to introduce this in core PRs without being disruptive. Perhaps exclude all files and start to gradually lint one file/module at a time(?).

For completeness, I will also try to get a PR going that uses JSHint, and we can compare which one we like the most. I don't think that I will bother creating a PR for JSLint though.