search

airbnb / javascript

JavaScript Style Guide
MIT License
145.5k stars 26.54k forks source link

What rules do the community tend to override? #1089

Open ljharb opened 8 years ago

ljharb commented 8 years ago

I'm opening this issue specifically to gather a list of the rules that those in the community feel are controversial or problematic, such that they override them, and most importantly, why.

This is not an invitation for "+1s" (those belong as reactions on individual posts), or an invitation to hear about everyone's subjective aesthetic preferences, nor is it an indication that any of these rules necessarily will - or will not - change. I'm simply hoping to primarily gather civil, well-reasoned explanations for why certain rules are overridden, disabled, etc.

Any comments mentioning spaces vs tabs, or semicolons, will be summarily deleted :-)

We're aware of the following:

  "import/no-extraneous-dependencies": ["error", {
    "devDependencies": true,
    "optionalDependencies": false,
  }],

Any others? I'll update the original post with new examples as I'm made aware of them.

(Note: this does not cover env settings, like "browser", "node", "mocha", etc - these are app-level concerns, and as such, your .eslintrc, tests/.eslintrc, etc should be defining them)

jonathanong commented 8 years ago
ljharb commented 8 years ago

@jonathanong Thanks! I'd forgotten camelcase, we have that problem too. Re arrow-body-style, would the existence of an autofixer make it not be a problem?

Can you elaborate on when prefer-template isn't convenient?

jonathanong commented 8 years ago

arrow-body-style - not sure, because sometimes i'd prefer having the entire { return x } block when the function is multi-line so that it's easy to add logic before the return in the future.

when i have something like path + '?' + query.

`${path}?${query}`

just seems like more work

ljharb commented 8 years ago

Thanks - I've added a few of your examples to the OP. (and fixed your markdown, hope you don't mind)

chadwatson commented 8 years ago

no-use-before-define for functions. I personally don't like to scroll down to the bottom of a file to see what a module really does. It's inevitable that for some modules it just makes sense to leave the helper functions in the same file. I prefer to see the declarative stuff first instead of having to scroll through the imperative stuff first.

steida commented 8 years ago

https://github.com/este/este/blob/master/.eslintrc

// Soft some rules.
"global-require": 0, // Used by React Native.
"new-cap": [2, {"capIsNew": false, "newIsCap": true}], // For immutable Record() etc.
"no-class-assign": 0, // Class assign is used for higher order components.
"no-nested-ternary": 0, // It's nice for JSX.
"no-param-reassign": 0, // We love param reassignment. Naming is hard.
"no-shadow": 0, // Shadowing is a nice language feature. Naming is hard.
"import/imports-first": 0, // Este sorts by atom/sort-lines natural order.
"react/jsx-filename-extension": 0, // No, JSX belongs to .js files
"jsx-a11y/html-has-lang": 0, // Can't recognize the Helmet.
"no-confusing-arrow": 0, // This rule is super confusing.
"react/forbid-prop-types": 0, // Este is going to use Flow types.
"react/no-unused-prop-types": 0, // Este is going to use Flow types.
"class-methods-use-this": 0, // Good idea, but ignores React render.
"arrow-parens": 0, // Not really.
ljharb commented 8 years ago

@steida Thanks!

Can you give me an example of your no-class-assign usage?

Also, fwiw, new-cap has capIsNewExceptions and newIsCapExceptions so you could define things like Record etc without having to alter the individual booleans.

Why would you need to disable forbid-prop-types or no-unused-prop-types if you're using Flow? It seems like they'd just be a noop if you don't have any propTypes defined.

As for class-methods-use-this, I added an exceptMethods option to the eslint rule, so the next version of the config will be excluding all the React lifecycle methods (including render), so you'll be able to remove that override soon. (update v12 of the config now handles this)

steida commented 8 years ago

As for no-class-assign https://github.com/este/este/blob/5571b8ab6722d5abd75984859292f5a5258c7151/src/browser/auth/Email.js#L155

As for forbid-prop-types and no-unused-prop-types I remember I had to set it, why? I don't remember.

As for the rest. Thank you.

ljharb commented 8 years ago

@steida thanks - for the prop types ones, please file bugs on eslint-plugin-react if you find you still need the overrides. for the no-class-assign one, it seems like you could do const InnerEmail = class Email { … } and then const FocusedEmail = focus(InnerEmail, 'focus'); etc?

jonase commented 8 years ago

The only custom rule we're using currently is

"new-cap": [2, {"capIsNewExceptions": ["Immutable.Map", "Immutable.Set", "Immutable.List"]}]
ljharb commented 8 years ago

@jonase I think those are common and legit enough that if you'd like to make a PR to the base config, I'd be happy to add them.

oliviertassinari commented 8 years ago

From Material-UI:

'no-console': 'error', // airbnb is using warn
'no-return-assign': 'off', // airbnb use error, handy for react ref assign.
'operator-linebreak': ['error', 'after'], // aibnb is disabling this rule

'react/jsx-handler-names': ['error', { // airbnb is disabling this rule
  eventHandlerPrefix: 'handle',
  eventHandlerPropPrefix: 'on',
}],
'react/jsx-filename-extension': ['error', {extensions: ['.js']}], // airbnb is using .jsx
'react/jsx-max-props-per-line': ['error', {maximum: 3}], // airbnb is disabling this rule
'react/no-danger': 'error', // airbnb is using warn
'react/no-direct-mutation-state': 'error', // airbnb is disabling this rule
bebraw commented 8 years ago

I have set the following at Reactabular:

"rules": {
  "comma-dangle": ["error", "never"], // personal preference
  "prefer-arrow-callback": 0, // mocha tests (recommendation)
  "func-names": 0, // mocha tests (recommendation)
  "import/no-extraneous-dependencies": 0, // monorepo setup
  "import/no-unresolved": [1, { ignore: ['^reactabular'] }], // monorepo setup
  "no-underscore-dangle": 0, // implementation detail (_highlights etc.)
  "no-unused-expressions": 0, // chai
  "no-use-before-define": 0, // personal preference
  "react/sort-comp": 0, // personal preference
  "react/no-multi-comp": 0 // personal preference
}
sompylasar commented 8 years ago 
"no-nested-ternary": 0,  // because JSX
"no-underscore-dangle": 0,  // because private properties
"arrow-body-style": 0,  // because cannot enforce what's personally preferred: 
  // (x) => ( some ? ( comple + x ) : ( e + x + pression ) )
  // (x) => some.trivialExpression()
  // (x) => { return ( <JSX /> ); }  // to add something later without re-indenting.
broguinn commented 8 years ago

At my company, we've overridden some rules that don't play nice with Mocha:

Also:

ljharb commented 8 years ago

@oliviertassinari Thanks! Most of yours are strengthening rules we don't have on/as errors. For ref assign, ref callbacks don't have a return value, so instead of ref => this.ref = ref, the guide recommends you use (ref) => { this.ref = ref; } which doesn't collide with the rule.

@bebraw re no-underscore-dangle, JS doesn't have private properties :-/ hopefully you've read the recent guide section on them. re no-unused-expressions, see below.

@broguinn arrow-body-style in mocha tests does do that, but i've found that using this in mocha tests is an antipattern, and an unfortunate API choice of mocha itself. For the few times we need it, we just use a normal function and give it a name. Re expect(true).to.be.true, that's a hugely problematic pattern, because expect(true).to.be.yogurt passes too - it's a noop. We recommend expect(true).to.equal(true) or expect(!!truthy).to.equal(true); which can't silently fail. Re prefer-rest-params, the guide does recommend/assume you are using babel.

To all: regarding no-nested-ternary, I understand why a single ternary statement is useful in JSX. Can someone provide me a code example of why a nested ternary is useful in JSX?

relekang commented 8 years ago

The interesting changes from our config is that we changed these to be able to use flowtype in a good way:

    "no-duplicate-imports": [0], // Does not support import of types from same file/module as is already used.
    "import/no-duplicates": [2], 
    "react/sort-comp": [2, {
      "order": [
        "type-annotations",  // sort type annotations on the top
        "static-methods",
        "lifecycle",
        "/^on.+$/",
        "/^(get|set)(?!(InitialState$|DefaultProps$|ChildContext$)).+$/",
        "everything-else",
        "/^render.+$/",
        "render",
      ],
    }],

You can find the config here. The other changes are mostly adding extra rules and disabling a few because we haven't always used airbnb as a base.

bebraw commented 8 years ago

@ljharb Yeah, I'm not using _ as private. It's a special case (convention) where I use it to signify a field with special meaning (i.e. something generated). I wouldn't go as far as to remove the rule from this preset.

ljharb commented 8 years ago

@relekang Thanks! re no-duplicate-imports, does Flow not support importing multiple things all in one import statement? import/no-duplicates has been enabled in our config since May, so you can safely remove it.

relekang commented 8 years ago

@ljharb I am not sure, but I have not found a way to combine 

import {someFunction} from 'module';

and

import type {someType, someOtherType} from 'module';

into one line.

import/no-duplicates has been enabled in our config since May, so you can safely remove it.

Nice 👌 Need to read the changelog closer.

ljharb commented 8 years ago

@relekang if that's true, i'd file that as a bug on flow :-)

devilleweppenaar commented 8 years ago

Thanks for soliciting community feedback @ljharb!

We override the following rules (that fall outside of the aesthetic preference category):

lelandrichardson commented 8 years ago

i'd argue that accepting duplicate import paths when one is import type and another is just import should probably be filed as a corner case with the import eslint plugin. Seems like a valid use case to me.

lencioni commented 8 years ago

It looks like there was an issue to support import type https://github.com/benmosher/eslint-plugin-import/issues/225 which has been fixed https://github.com/benmosher/eslint-plugin-import/pull/334. The docs for the rule claim to support this, too, so I think you should be good to go!

There is an open issue about allowing exporting types from import/prefer-default-export: https://github.com/benmosher/eslint-plugin-import/issues/484

relekang commented 8 years ago

@ljharb @lelandrichardson @lencioni I did not mean to say that the rule from the import plugin did not work. I tried to say that the rule no-duplicate-imports from eslint itself gives what I would think of as false positives with flowtype imports. Is there a reason that both are in use here? It looks like the one from the import plugin covers the cases that the one from eslint does.

no-duplicate-imports is turned on in es6.js

ljharb commented 8 years ago

@relekang @lencioni @lelandrichardson since no-duplicate-imports is an eslint core rule, they'd need to support flow in order to detect that corner case. I think it would make more sense for Flow to allow you to combine import types and imports all into one statement.

jcreamer898 commented 8 years ago

@ljharb

I'm a little confused about the react/forbid-prop-types change, why is it bad to pass an object?

What's the alternative?

Thanks!

ljharb commented 8 years ago

@jcreamer898 it's not specific enough - use shape or objectOf (and arrayOf over array, etc)

jcreamer898 commented 8 years ago

Ahhh, objectOf was what I was missing. Thanks!

vdh commented 8 years ago

I like to lean towards accepting Airbnb's rules (despite what this lengthy list seems to indicate), so any chance to reduce my own overrides list would be great 😃 

---
rules:
  import/no-extraneous-dependencies:
    - error
    -
      devDependencies: true
  curly:
    - error
    - all
  new-cap:
    - error
    -
      newIsCap: true
      capIsNewExceptions:
        # Allow Immutable.js classes
        - List
        - Map
        # ...etc
  no-unused-vars:
    - warn
    -
      vars: all
      args: all
      varsIgnorePattern: ^_
      argsIgnorePattern: ^_

  # Downgraded to warnings
  no-unused-vars: warn
  no-useless-concat: warn
  prefer-template: warn
  semi: warn
  spaced-comment: warn
  react/prefer-stateless-function: warn

  # Disabled
  no-mixed-operators: off
  no-continue: off
  no-plusplus: off
  react/no-children-prop: off
  react/no-unescaped-entities: off
  react/forbid-prop-types: off
  react/no-unused-prop-types: off
  jsx-a11y/no-static-element-interactions: off
ljharb commented 8 years ago

@vdh Thanks! Some responses:

AlicanC commented 8 years ago 
// Imports that introduce global effects. The same ordering below applies to this.
import 'babel-polyfill';
import './appStyle.css';

// Third-party modules.
import React from 'react';
import _ from 'lodash';

// Shared code. Sourced in the project, but not specific to current component
import CoolButton from 'components/CoolButton'; // webpack alias
import CoolUtils from '../utils/CoolUtils'; // Upper directory

// Private code. Also sourced in the project, but are specific to current component and never required from anywhere else
import Styles from './styles';
import TabOne from './tabs/TabOne';
import TabTwo from './tabs/TabTwo';
import Foo from 'foo';
const Bar = (Platform.OS === 'ios') ? require('bar-ios') : null;
import Baz from 'baz';
function myFunc() {
  const foo = ...;
  if (!foo) return;

  const bar = ...;
  if (!bar) return;

  const baz = ...;
  if (!baz) return;

  // There is no reason to stop, do it
  doWork();
}

We want to apply the same style to loops, but without continue, we end up with indentation-hell:

for (const thing of things) {
  const foo = ...;
  if (foo) {
    const bar = ...;
    if (bar) {
      const baz = ...;
      if (baz) {
        doWork();
      }
    }
  }
}
ljharb commented 8 years ago

@AlicanC thanks! The only ones I think need commenting are underscore (there's no such thing as "privates"); imports-first: imports are hoisted, so your conditional import will run after every static one anyways - the rule enforces that the code's order matches evaluation order; and no-continue: the guide prohibits any loops at all, so this shouldn't come up (I'd recommend filter chains on things for example)

knpwrs commented 8 years ago

As far as import/no-extraneous-dependencies is concerned, the rule itself supports glob configuration: https://github.com/benmosher/eslint-plugin-import/blob/2dec0a7f33b737371efb7acd60c8a30a48562ccb/docs/rules/no-extraneous-dependencies.md

vdh commented 8 years ago

@ljharb

export const sizeShape = process.env.NODE_ENV === 'production' ? null : /* ... */
ljharb commented 8 years ago

@vdh if you're not passing children inside jsx, such that you get a closing tag, what's the point of jsx? "children" is special because React.createElement accepts them as a third argument, and because they can be used as actual jsx descendants (altho you're correct that they end up identical inside react).

oliviertassinari commented 8 years ago

Another point as nobody raised it yet. jsx-boolean-value is enforced to ['error', 'never']. But don't we lose expressivity? It seems than @zpao is against it.

ljharb commented 8 years ago

@oliviertassinari jsx is html, and this is another similarity to html. In general explicit is much better than implicit, but this is one of the many HTML vagaries you can't get away with not knowing anyways.

sompylasar commented 8 years ago

JSX boolean rule makes sense because one case is expressed in strictly one way: true without value, false with value. Otherwise true may be expressed in two ways. There might be a rule that forbids the other way, i.e. true without the value, but I haven't seen such.

vdh commented 8 years ago

@ljharb I still don't understand this vilification of the children prop. It was proposed and added with little explanation or reasoning in its documentation aside from the fact that it's "special" because of how JSX compiles nesting into the third argument of React.createElement. It's very arbitrary.

If you're not passing children inside jsx, such that you get a closing tag, what's the point of jsx?

Isn't the point of JSX to be a syntactic sugar to nest multiple levels of components inside children props? If I'm at a "leaf" node, why do I need to enforce the same nesting syntax on my non-component values? <a> tags can have further nesting, but if I write <a children="Foo" /> this is a visual indicator that this particular <a> tag is currently being used as a leaf (and although they are technically another nested node, text nodes are very rarely treated as such).

I've even found a previous discussion where someone suggested removing it from the resulting props altogether, but the end result of that discussion seems to be that it is not that special. It's just another prop. It's only been special in the context of validation and rendering. To quote the first reply in that issue:

This seems odd to me, I agree that the way children is currently implemented is weird, but IMHO that's not because it doesn't belong in props. It's because React.createElement provides the convenience of specifying children through varargs, which to me is a convenience provided for non-JSX users but otherwise not a good API decision.

I'm sorry to be so stubborn about this, but I've searched for and found no evidence in the React docs or any comments from the React developers discouraging the use of passing children via prop. If avoiding it was an official stance, or if there was a real technical hurdle to using it, I would drop this. But I don't like the idea of just arbitrarily banning something without technical merit.

ljharb commented 8 years ago

@vdh it's not an official stance of the React team, that's for sure. The issue is that without nested children, we should be using React.createElement calls instead of jsx - there is a large technical cost to using a non-standard-JS syntax. The primary arguments for using JSX is that when nesting children, the closing element (like XML/HTML) contains the opening element's name, which makes nesting more visually identifiable than a closing paren. In other words, I'm claiming that if you're not nesting children anywhere, then you probably shouldn't be using JSX anywhere either. Let's open a new issue though if you want to discuss this further, since the point of this issue is to discuss which rules are overridden, and NOT to debate specific rules.

ljharb commented 8 years ago

@knpwrs thanks - eslint-plugin-import started supporting globs for that rule in v1.15.0, so I'll update eslint-config-airbnb-base soon with that enabled.

kevinSuttle commented 8 years ago 
 "rules": {
    "prefer-template": 2,
    "comma-dangle": [2, "always-multiline"],
    "object-curly-spacing": [
      "error",
      "always"
    ],
    "import/extensions": [2, "never", { "js": "never", "jsx": "never", "json": "never"}],
    "import/newline-after-import": 0,
    "import/no-extraneous-dependencies": [
      "off",
      {
        "devDependencies": false,
        "optionalDependencies": false
      }
    ],
    "react/jsx-filename-extension": [1, { "extensions": [".js", ".jsx"] }]
  }

An easy way to find out: https://medium.com/google-cloud/static-javascript-code-analysis-within-bigquery-ed0e3011732c

ljharb commented 8 years ago

@kevinSuttle prefer-template, comma-dangle, and object-curly-spacing are already defined in this config, so you don't need to override those. The others are pretty straightforward, thanks!

milesj commented 8 years ago

I turn these off:

And then I have "no-unused-vars": [2, { "vars": "all", "args": "none" }], which is primarily to support flowtype.

I also disable a handful more for tests, as it makes writing tests more cumbersome.

{
  "rules": {
    "max-len": 0,
    "no-undef": 0,
    "func-names": 0,
    "prefer-arrow-callback": 0,
    "import/no-extraneous-dependencies": 0,
    "import/prefer-default-export": 0
  }
}
ljharb commented 8 years ago

@milesj can you show me an example of when it's a scalar/primitive that you think it's OK?

For tests, you should use "import/no-extraneous-dependencies": [2, { "devDependencies": true }] for safety.

milesj commented 8 years ago

Something like this, very rudimentary.

function update(data, action = true) {
  if (condition) {
    action = false;
  }

  // ...
}
jkeljo commented 8 years ago

no-mixed-operators - allowSamePrecedence: true. Having to separate arithmetic operators of the same precedence was unnecessary work. The linter still helps you get it right even for operators whose precedence is less commonly understood, because if it hasn't made you put parens anywhere you know they must have been at the same precedence.

no-plusplus - allowForLoopAfterthoughts: true. The guide forbids loops. But in places where I use one anyway, the afterthought of a for loop is immune to the semicolon insertion problem that motivates the no-plusplus rule to begin with.

no-underscore-dangle - yes, putting an underscore does not a private field make. Yet the Crockford pattern or WeakRef is a lot of ugliness that I'm not going to bother with in most cases. I find I'd rather have a hint than nothing. There's at least one Babel transform out there to make underscores really private, but I haven't gone there yet.

react/jsx-no-bind - I tighten this to forbid arrow functions and not ignore refs, because the former can cause shouldComponentUpdate implementations to erroneously return true, and the latter results in a lot of extra calls to the ref callback.

react/sort-comp - the default order and the style guide order both just feel weird. Can't put all props at the top, or put render and its helper methods in reading order, for example.

I also turn off some rules that either don't play nice with Flow or where Flow does a better job than the linter at detecting the problem.

AlicanC commented 8 years ago

no-unused-vars is also problematic in Express applications. callback.length must be 4 for a callback to be considered an error handler:

// We won't use "next" but we can't omit it
app.use((err, req, res, next) => { /* ... */ });
// If we omit it we get `req, res, next` instead of `err, req, res`:
app.use((err, req, res) => { /* err is req, req is res, res is next and you are f'd */ });

I don't override this rule though, I just disable it for the line. The problem is not the rule, it's Express' API.

I might override it in the future since I like to keep unused arguments as documentation.

gaurav21r commented 8 years ago

Thank you @ljharb for this issue, its good to know you are interested in hearing out the community even in cases where your stand is clear.

I just thought dangling underscores no-underscore-dangle deserves a mention of its own . #490 has been around for a year. 6 comments above, have them in their overrides.

Your stance on the fact that JavaScript has no real private properties is well taken! However I think most people are using this convention because the framework of their choice uses this convention. Eg: Polymer. Whether or not it is the right thing to do, it is highly prevalent in the Javascript industry as of today.