javascript

javascript copied to clipboard

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:

• react/jsx-filename-extension - filed as #985. Why is it controversial? Some believe that .js files should be allowed to contain JSX. Others believe this is an app-level concern, and does not belong in a shared config (like "env" settings).
• ~~import/no-extraneous-dependencies: in test directories, for example, it needs to be overridden to the following (note: if/when eslint allows glob-based config, then we'd set up some common test directory patterns so you wouldn't have to do this manually)~~ eslint-config-airbnb v13.0.0 is now released, which resolves this.

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

• camelcase: when you're bound to the casing selected by APIs (https://github.com/airbnb/javascript/issues/1089#issuecomment-249396262, but Airbnb suffers from this too) This might also apply to new-cap and no-underscore-dangle.
• no-param-reassign: this happens frequently with a reduce where the entire operation is pure, but the reducer is not (ie, it mutates the accumulator, but the accumulator began as {} passed into the reducer). You can use Object.assign({}, accumulator, changes), or { ...accumulator, ...changes }, however. (per https://github.com/airbnb/javascript/issues/1089#issuecomment-249396262)
• no-use-before-define: some enjoy using hoisting to define helper methods at the bottom of the file. This guide discourages relying on hoisting; instead suggesting importing the helpers from another file when possible. (per https://github.com/airbnb/javascript/issues/1089#issuecomment-249396262)
• no-mixed-operators - specifically where it relates to arithmetic operators, where the PEMDAS rule applies. We're definitely considering loosening this rule as it applies to *, /, +, and -.

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)

• arrow-body-style or whatever, because it makes converting regular functions to arrow functions difficult as it's not fixed automatically.
• camelcase - because many times i'm bound by other APIs
• no-param-reassign - makes working with koa/express/node.js difficult
• no-plusplus - because i += 1 is annoying
• prefer-template - because it isn't convenient 5% of the time
• consistent-return - annoying with early returns
• new-cap - bound by other APIs
• no-underscore-dangle - disallows "private" properties. bound by mongodb.
• no-use-before-define - disallows function declarations at the bottom

@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?

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

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

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.

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.

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

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.

@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?

The only custom rule we're using currently is

"new-cap": [2, {"capIsNewExceptions": ["Immutable.Map", "Immutable.Set", "Immutable.List"]}]

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

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

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
}

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

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

• arrow-body-style means before hooks can't access the all-important this
• no-unused-expressions makes expect(true).to.be.true; to cause a linting error

Also:

• prefer-rest-params is not supported in Node 4.x

@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?

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.

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

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

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

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

Thanks for soliciting community feedback @ljharb!

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

• require-yield: 0 - Working with Koa.js, there are some cases where we don't yield from a generator.
• strict: [0, "global"] - We work mostly with ES6 modules, which are strict already.

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.

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

@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

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

@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!

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

Ahhh, objectOf was what I was missing. Thanks!

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

• import/no-extraneous-dependencies: We keep our build tools and development-only deps in devDependencies.
• curly: I find if (foo) { return 'bar'; } to be less ambiguous than if (foo) return 'bar'; for single-line blocks.
• no-mixed-operators: I use a && b || c all the time for short-circuit evaluation, and requiring brackets for a + b * c feels like a betrayal of everything I learned in high-school algebra. Logical or arithmetic operators feel too core to be putting heavy-handed bracket requirements over them.
• no-continue: continue is useful when controlling loop flow, and a lot of function-based loops use return as a substitute for it, so why disallow the real thing?
• no-plusplus: Unary operators are common for loop structures, not every unary needs to be expanded to += 1. If there was a rule to restrict unary to only the inside of loop blocks, or to disallow whitespace around unary operators (It feels like a huge mistake to let that be a valid syntax), I might like that.
• react/no-children-prop: <a children="foo" /> feels just as valid as <a>foo</a> is when children are simple values like strings.
• react/no-unescaped-entities: This one feels like it's crippling the wonderful built-in escaping already provided by React, just to provide some typo catching. I would actually prefer an opposite no-escaped-entities rule that would forbid HTML entities entirely.
• react/forbid-prop-types: I really like the sentiment behind this one, to enforce descriptive prop types, but it really hampers prop inheritance. E.g. if a <Page> component is going to pass a user object down to a <Header> component that has the descriptive shape prop type validation. <Page> never uses user directly, so it only has to validate that user is an object, and the more in-depth validation is handled by <Header>.
• react/no-unused-prop-types: Problematic when handling shape, and also has trouble detecting when a prop is used outside of render, or passed down to child components via prop inheritance.
• jsx-a11y/no-static-element-interactions: Also one that I like the sentiment for, but some designs I'm given it's sometimes extremely awkward to attempt to avoid click events for static elements. If this rule could be split on mouse vs keyboard events, I would love to enforce no static elements for keyboard events.
• react/prefer-stateless-function: Usually arrives before the class is actually finished being written, so the downgrade to warning is to make it less persistent.
• no-unused-vars, no-useless-concat, prefer-template, semi, spaced-comment: Downgraded to warnings because they're style-related rules that can be dealt with after more important lint errors.
• new-cap: Already covered earlier, and I think my shorthand use of Immutable classes is too niche to worry about, especially since I'm shadowing the ES6 Map.
• no-unused-vars: Also might be too niche, using a _ prefix for purposefully-unused vars and arguments.