Site-kit-wp: Fix ESLint warnings

Created on 12 Sep 2019  ·  9Comments  ·  Source: google/site-kit-wp

Bug Description

Currently the build emits many eslint warnings, mostly related to missing JSDoc elements.

This was introduced by the update to the configuration in #217. While these warnings do not fail the build, they indicate areas which require attention and should be fixed.

Steps to reproduce

  1. Run npm install
  2. Run npm run lint:js

Screenshots

image


_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

  • To prevent us from omitting documentation in the future, the eslint configuration should be modified so that all warnings from here should instead be flagged as errors.
  • All eslint warnings/errors emitted during build should be addressed (mostly revolving around missing function documentation).

Implementation Brief

  • Update @wordpress/eslint-plugin to ^3
  • Update eslint-loader to ^3

    • Set options.quiet to true

      This lowers the verbosity of eslint during a build. Without this, errors are emitted and reported at the end, resulting in a firehose of output and duplicate messages.

      This was done as part of #993

  • Manually fix all linting errors and warnings
    Almost all of them are for missing @return annotations

Changelog entry

  • N/A
P2 Eng Bug

Most helpful comment

I would recommend updating @wordpress/eslint-plugin from 2.4 to 3.1 before doing this.

Why? In version 3, the plugin changed its config from using the deprecated built-in JSDoc rules to stricter JSDoc linting using eslint-plugin-jsdoc.

Otherwise you'd fix these warnings here and then later have to do some fixes _again_ because of that change.

Also, the package has other relevant bug fixes and enhancements: https://github.com/WordPress/gutenberg/blob/5e854c2f9b4d58914340b64851e7622c65f0d270/packages/eslint-plugin/CHANGELOG.md

Oh, and once these issues are fixed, I'd make sure that JSDoc rules are treated as errors and not warnings.

All 9 comments

@aaemnnosttv Are you talking about fixing the respective code (mostly docs), or do you mean the warnings should be suppressed?

The code should be updated to fix the warnings, not suppress them 😄

I would recommend updating @wordpress/eslint-plugin from 2.4 to 3.1 before doing this.

Why? In version 3, the plugin changed its config from using the deprecated built-in JSDoc rules to stricter JSDoc linting using eslint-plugin-jsdoc.

Otherwise you'd fix these warnings here and then later have to do some fixes _again_ because of that change.

Also, the package has other relevant bug fixes and enhancements: https://github.com/WordPress/gutenberg/blob/5e854c2f9b4d58914340b64851e7622c65f0d270/packages/eslint-plugin/CHANGELOG.md

Oh, and once these issues are fixed, I'd make sure that JSDoc rules are treated as errors and not warnings.

@felixarntz it would be great to revisit this one again sometime soon.

IB ✅

This one needs a bit more work to finish. @wordpress/eslint-plugin v4 was just released which we should aim to use now since 3.x may not get further updates. v4 has more of an integration with Prettier which introduces many changes in lint rules. Gutenberg adds a .prettierrc which proxies the config from @wordpress/scripts https://github.com/WordPress/gutenberg/blob/5fde188a8c3b7603fbb29fca7df56ba34a06b1ae/.prettierrc.js but this isn't added until scripts v7 (we're a bit behind here as well).

@tofumatt will take a look at adapting our rules to work with the newer rulesets with select overrides to allow for more of an incremental change as to avoid a huge diff and merge conflict hell.

v4 has more of an integration with Prettier which introduces many changes in lint rules

There is also a ruleset without any Prettier formatting at all, FWIW.

Thanks @swissspidy - I recall seeing something about that since my last comment as well. I'll take another look. It might be a good idea to adopt Prettier in the future, but opting out of that at first is probably the smaller+safer incremental change 👍

QA ✅

This didn't seem to affect the app at all; many changes are documentation but the non-doc changes seem fine too.

JSDoc errors now throw warnings, and helpfully no errors/warnings are thrown by default.

Was this page helpful?
0 / 5 - 0 ratings