Cleanup monorepo linting configuration #758

edmorley · 2018-03-22T01:33:39Z

Fixes some of the issues found whilst looking at #608.

See individual commits/commit messages for more details.

edmorley · 2018-03-22T01:50:34Z

Another issue not dealt with here (since it requires hundreds of fixes) is that whilst the prettier and prettier/react presets are being used to turn off conflicting AirBnB rules, the actual prettier rules to replace them aren't enabled. To do so requires adding:

        rules: {
          // ...
          'prettier/prettier': 'error'
        }

See:
https://github.com/prettier/eslint-plugin-prettier#installation

eliperelman · 2018-03-22T15:40:06Z

.neutrinorc.js

@@ -1,103 +1,37 @@
 module.exports = {
  use: [
-    ['./packages/airbnb-base', {
+    ['./packages/airbnb', {


As mentioned in the other comment, we don't actually use any react in the repo, and aren't linting the create-project JSX files in this repo using our own configuration. In my opinion, I think this should stay as airbnb-base.

If that's the case, we should remove the react eslint plugin references from .neutrinorc.js then - since we are already using a React based linting config - this change was keeping the status quo.

That said, I wonder if we should actually eventually remove the create-project templates from .eslintignore and start linting them like we do the rest of the repository. Yes we'll still need the one off lint step during create-project bootstrap, but at least we'd catch/fix more obvious issues in the code we have checked in - plus it would improve consistency with the rest of the monorepo from a code style/best practices perspective.

but at least we'd catch/fix more obvious issues in the code we have checked in

I think this is a compelling reason to at least lint it ourselves...somehow. It would be nice to catch eslint errors at patch-time instead of finding them in release. Stylistic fixes will still be done during create-project.

I was hoping the eslint overrides feature would let us do this, however it seemed like it does not work via the ESLint API, and only via the CLI sadly.

eliperelman · 2018-03-22T15:40:16Z

.neutrinorc.js

@@ -1,103 +1,37 @@
 module.exports = {
  use: [
-    ['./packages/airbnb-base', {
+    ['./packages/airbnb', {
+      // Excludes are managed vis `.eslintignore` since `exclude` doesn't support globs.


eliperelman · 2018-03-22T15:40:51Z

.neutrinorc.js

-          'callback-return': 'off',
-          // Allow using class methods with static/non-instance functionality
-          // React lifecycle methods commonly do not use an instance context for anything
-          'class-methods-use-this': 'off',
          // Disallow trailing commas on arrays, objects, functions, et al
          'comma-dangle': ['error', 'never'],


I'm fine with removing this if you like.

Ah so when I looked into comma-dangle it was a bit confusing. Airbnb enables it as 'always', the eslint-config-prettier preset then turns it off entirely, and ordinarily the 'prettier/prettier' rule would then turn it back on as 'never' - but we're not using 'prettier/prettier' at the moment.

So removing this line would make it 'comma-dangle': 'off'. (Which may or may not be what you meant?) :-)

Grr, I'm dumb. I guess I was trying to align with:

https://github.com/mozilla-frontend-infra/neutrino-preset-mozilla-frontend-infra/pull/16/files#diff-5c42a4edfb59f1918fbdad6b1fbd9ba2R57

Thoughts?

Zooming out a bit, my thoughts are:

I like the idea of Neutrino dogfooding it's own linting configs

To get the most value of dogfooding we should limit the amount we override them (and if things really are an issue, fix the preset directly)

If we want to use prettier, perhaps medium-long term we should create/use a prettier neutrino preset

Long term I'd like to see most of the style rules in neutrino-preset-mozilla-frontend-infra go away (I don't really think they belong there - ideally projects should use stock presets and not deviate too much. And I don't think that preset should turn off things like react/prefer-stateless-function - the individual projects should make those choices IMO)

And I don't think that preset should turn off things like react/prefer-stateless-function - the individual projects should make those choices IMO

To clarify on this part - I agree that for legacy projects some rules (eg react/prop-types or react/require-default-props) are too burdensome - however by hiding the setting of it to 'off' in neutrino-preset-mozilla-frontend-infra, it means brand new projects (which have no reason to not do things right from the start) don't know to turn it on.

Good points all around. Most of the rules in our preset are stylistic choices to improve readability/maintainability, prevent mutation bugs, or get in the way of doing FP.

Let's raise these as issue in that preset.

Fwiw the AirBnB style guide are sometimes receptive to suggestions for rule changes. By filing issues against them and linking to those issues when making adjustments, it would (a) show that upstreaming has been attempted, (b) give breadcrumbs to follow for understanding why the deviation :-)

Yeah, I should also try and document why we currently have those deviations in the interim. (Part of my master plan for our org styleguide, which I haven't done yet).

eliperelman · 2018-03-22T15:41:18Z

packages/create-project/commands/init/index.js

@@ -10,7 +10,7 @@ const { projects, packages, isYarn } = require('./utils');

 /* eslint-disable no-underscore-dangle */
 module.exports = class Project extends Generator {
-  _logo() {
+  static _logo() {


edmorley · 2018-03-22T16:27:41Z

I've made the s/vis/via/ change. Regarding airbnb vs airbnb-base, should I:

leave the PR as is
update the PR to use airbnb-base again, but still remove the react plugin/rule references etc

eliperelman · 2018-03-22T16:40:07Z

update the PR to use airbnb-base again, but still remove the react plugin/rule references etc

Probably this, so it's not confusing that it's not doing anything right now.

Since we don't currently lint any React files, and if we start doing so in the future we should use the airbnb preset instead of airbnb-base.

These rules are now either set via the AirBnb or prettier presets, or else are no longer required. Fewer overrides makes it easier to reason about the actual differences between AirBnB/prettier and the linting config used for this monorepo. The only code fix required was making `_logo()` be a static method in the create-project init command, to fix: https://eslint.org/docs/rules/class-methods-use-this

Switches from opt-in to opt-out to avoid accidentally omitting new directories or non-JS file types. The list of files scanned is the same before/after (confirmed using `yarn lint --debug`).

This prevents warnings during yarn install of the monorepo: ``` warning " > eslint-config-prettier@2.9.0" has unmet peer dependency "eslint@>=3.14.1". ```

edmorley self-assigned this Mar 22, 2018

edmorley requested a review from eliperelman March 22, 2018 01:56

edmorley mentioned this pull request Mar 22, 2018

Fix monorepo "unmet peer dependency" warnings #759

Closed

eliperelman reviewed Mar 22, 2018

View reviewed changes

edmorley added 4 commits March 22, 2018 17:00

Remove React presets/rules from monorepo lint config

11ab7ea

Since we don't currently lint any React files, and if we start doing so in the future we should use the airbnb preset instead of airbnb-base.

Make monorepo linting file includes less fragile

acc4d72

Switches from opt-in to opt-out to avoid accidentally omitting new directories or non-JS file types. The list of files scanned is the same before/after (confirmed using `yarn lint --debug`).

Add eslint to monorepo root devDependencies

78900e4

This prevents warnings during yarn install of the monorepo: ``` warning " > eslint-config-prettier@2.9.0" has unmet peer dependency "eslint@>=3.14.1". ```

edmorley requested a review from eliperelman March 22, 2018 17:22

eliperelman approved these changes Mar 23, 2018

View reviewed changes

edmorley merged commit 9473b74 into neutrinojs:master Mar 23, 2018

edmorley deleted the cleanup-lint-config branch March 23, 2018 15:40

edmorley added the development label Jul 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup monorepo linting configuration #758

Cleanup monorepo linting configuration #758

edmorley commented Mar 22, 2018

edmorley commented Mar 22, 2018 •

edited

Loading

eliperelman Mar 22, 2018

edmorley Mar 22, 2018 •

edited

Loading

eliperelman Mar 22, 2018

edmorley Mar 22, 2018 •

edited

Loading

eliperelman Mar 22, 2018

eliperelman Mar 22, 2018

edmorley Mar 22, 2018

eliperelman Mar 22, 2018 •

edited

Loading

edmorley Mar 22, 2018

edmorley Mar 22, 2018 •

edited

Loading

eliperelman Mar 22, 2018

edmorley Mar 22, 2018

eliperelman Mar 22, 2018 •

edited

Loading

eliperelman Mar 22, 2018

edmorley commented Mar 22, 2018

eliperelman commented Mar 22, 2018

Cleanup monorepo linting configuration #758

Cleanup monorepo linting configuration #758

Conversation

edmorley commented Mar 22, 2018

edmorley commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

edmorley Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmorley Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmorley Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmorley commented Mar 22, 2018

eliperelman commented Mar 22, 2018

edmorley commented Mar 22, 2018 •

edited

Loading

edmorley Mar 22, 2018 •

edited

Loading

edmorley Mar 22, 2018 •

edited

Loading

eliperelman Mar 22, 2018 •

edited

Loading

edmorley Mar 22, 2018 •

edited

Loading

eliperelman Mar 22, 2018 •

edited

Loading