Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Cleanup monorepo linting configuration #758

Merged
merged 4 commits into from
Mar 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
packages/**/node_modules/
# TODO: Fix lint errors in our tests and remove this line
packages/**/test/
# The templates have any lint errors fixed during project creation,
# since we don't know which ESLint preset the user will choose.
packages/create-project/commands/init/templates/
77 changes: 5 additions & 72 deletions .neutrinorc.js
Original file line number Diff line number Diff line change
@@ -1,103 +1,36 @@
module.exports = {
use: [
['./packages/airbnb-base', {
// Excludes are managed via `.eslintignore` since `exclude` doesn't support globs.
include: [
'.*.js',
'packages/*/*.js',
'packages/*/{src,bin,commands}/*.js',
'packages/*/commands/init/*.js',
'packages/neutrino/bin/*'
'packages/'
],
eslint: {
baseConfig: {
extends: [
'plugin:react/recommended',
'prettier',
'prettier/react'
'prettier'
]
},
envs: ['browser', 'commonjs', 'node'],
plugins: [
'eslint-plugin-prettier',
'eslint-plugin-react'
'prettier'
],
rules: {
// Algebraic and functional types should allow capital constructors without new
'babel/new-cap': 'off',
// Disable necessitating return after a callback
'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'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with removing this if you like.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eliperelman eliperelman Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@edmorley edmorley Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@eliperelman eliperelman Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Require all requires be top-level
// http://eslint.org/docs/rules/global-require
'global-require': 'error',
// Enforces error handling in callbacks (node environment)
'handle-callback-err': 'off',
// Allow dynamic requires
'import/no-dynamic-require': 'off',
// Keep all the original Airbnb indentation rules except MemberExpression,
// which messes up our pretty chaining indentations
indent: ['error', 2, {
SwitchCase: 1,
VariableDeclarator: 1,
outerIIFEBody: 1,
MemberExpression: 'off',
FunctionDeclaration: {
parameters: 1,
body: 1
},
FunctionExpression: {
parameters: 1,
body: 1
},
CallExpression: {
arguments: 1
},
ArrayExpression: 1,
ObjectExpression: 1,
ImportDeclaration: 1,
flatTernaryExpressions: false,
ignoredNodes: ['JSXElement *']
}],
// Specify the maximum length of a line in your program
// JSX can get lengthy, so this helps alleviate that a bit
// http://eslint.org/docs/rules/max-len
'max-len': ['error', 120, 2, {
ignoreUrls: true,
ignoreComments: false,
ignoreStrings: true,
ignoreTemplateLiterals: true
}],
// Allow console during development, otherwise throw an error
'no-console': 'off',
// Allow extra parentheses since multiline JSX being wrapped in parens is considered idiomatic
'no-extra-parens': 'off',
// Disallow mixing regular variable and require declarations
'no-mixed-requires': ['off', false],
// Disallow use of new operator with the require function
'no-new-require': 'error',
// Disallow string concatenation with __dirname and __filename
// http://eslint.org/docs/rules/no-path-concat
'no-path-concat': 'error',
// Allow use of process.env
'no-process-env': 'off',
// Allow process.exit()
'no-process-exit': 'off',
// Restrict usage of specified node modules
'no-restricted-modules': 'off',
// Allow return assign
'no-return-assign': 'off',
// Allowing shadowing variable that share the same context as the outer scope
'no-shadow': 'off',
// It makes sense to have unused expressions to avoid imperative conditionals
'no-unused-expressions': 'off',
// Allow use of synchronous methods (off by default)
'no-sync': 'off',
// Our frontend strives to adopt functional programming practices, so we prefer const over let
'prefer-const': 'error'
'no-unused-expressions': 'off'
}
}
}]
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
"auto-changelog": "^1.2.2",
"ava": "^0.25.0",
"codecov": "^3.0.0",
"eslint": "^4.12.1",
"eslint-config-prettier": "^2.9.0",
"eslint-plugin-prettier": "^2.3.1",
"eslint-plugin-react": "^7.5.1",
"gh-pages": "^1.1.0",
"gitbook-cli": "^2.3.2",
"gitbook-plugin-anchorjs": "^1.1.1",
Expand Down
4 changes: 2 additions & 2 deletions packages/create-project/commands/init/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const { projects, packages, isYarn } = require('./utils');

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

return ` _ _
_ __ ___ _ _ | |_ _ __ (_) _ __ ___
| '_ \\ / _ \\| | | || __|| '__|| || '_ \\ / _ \\
Expand Down Expand Up @@ -101,7 +101,7 @@ module.exports = class Project extends Generator {
prompting() {
const done = this.async();

this.log(chalk.cyan.bold(this._logo()));
this.log(chalk.cyan.bold(Project._logo()));
this.log(chalk.white.bold('Welcome to Neutrino! 👋'));
this.log(chalk.cyan('To help you create your new project, I am going to ask you a few questions.\n'));

Expand Down