[Core] Improve eslint a bit more

[oliviertassinari]

Having a look at the source code, I think that we can stil improve it before starting doing heavy changes.
I was thinking about enforcing those rules:

• brace-style: 2: that prevent merge conflict between / coding style [eslint] Enforce brace-style #2920
• react/jsx-pascal-case: 2 https://github.com/airbnb/javascript/tree/master/react#naming [eslint] Enforce new react rules #2953
• react/jsx-closing-bracket-location: 2 [eslint] Enforce new react rules #2953
• react/jsx-max-props-per-line: [2, {maximum: 3}] [eslint] Enforce new react rules #2953
• jsx-equals-spacing: 2 [eslint] enforce react/jsx-equals-spacing #3035
• arrow-parens: 2 [eslint] Enforce arrow-parens #3207
• prefer-template: 2 [eslint] Enforce prefer-template #3242
• no-unneeded-ternary: 2 [eslint] Enforce one more rule no-unneeded-ternary #3320
• prefer-const: 2: that reduce the overhead when trying to understand what the property is for. [eslint] Refactor some component to use const instead of let #3300 [eslint] Enforce prefer-const #3315
• jsx-space-before-closing: 2 [eslint] Enforce jsx-space-before-closing #3397
• id-blacklist: [2, e] [eslint] Move from e to event #3398
• react/no-is-mounted: 2 [Core] Remove isMounted() #3437
• padded-blocks: [2, never] [eslint] enforce padded-blocks never #3493
• no-multiple-empty-lines: 2 [eslint] Enforce operator-linebreak and no-multiple-empty-lines #3516
• operator-linebreak: [2, "after"] [eslint] Enforce operator-linebreak and no-multiple-empty-lines #3516
• react/jsx-handler-names: 2 [eslint] Enforce jsx-handler-names #3408
• react/prefer-es6-class: 2
• prefer-arrow-callback: 2: well, because it's cool?
• react/no-multi-comp: 2: that's simpler to look for a specific component
• no-shadow: 2

[alitaheri]

Are these all that's left?

[oliviertassinari]

I was also thinking about:

• consistent-return: 2
• no-shadow: 2
• new-cap: [2, {capIsNew: true, newIsCap: true}]

But I'm not 100% convinced we need them.

[alitaheri]

no-shadow is good to have. It can actually prevent some hideous bugs that can be caused by renaming inner function arguments. And I think there can be at most one or two occurrence of this

[newoga]

I think these are all mostly good. I could definitely argue for some of them and I can't really argue against any of them. I'm open to what you both think is best.

At some point I would be curious to do a comparison of our eslint feature set to React's or AirBnb's. For example, is our's looser or stricter? And if we happen to be really close to one, maybe we could adopt it. If we're not close but feel strongly about ours, then we can leave it be.

[newoga]

We should finish discussing our stance on object-shorthand at some point.

[newoga]

I think we should add jsx-no-bind when we're able to: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md

Should be pretty simple:

src/auto-complete.jsx
  408:13  error  JSX props should not use arrow functions  react/jsx-no-bind
  414:13  error  JSX props should not use arrow functions  react/jsx-no-bind
  418:13  error  JSX props should not use arrow functions  react/jsx-no-bind
  422:13  error  JSX props should not use arrow functions  react/jsx-no-bind

src/DropDownMenu/DropDownMenu.jsx
  388:11  error  JSX props should not use .bind()  react/jsx-no-bind

src/time-picker/clock.jsx
  180:11  error  JSX props should not use .bind()  react/jsx-no-bind
  181:11  error  JSX props should not use .bind()  react/jsx-no-bind

src/time-picker/time-display.jsx
  151:11  error  JSX props should not use arrow functions  react/jsx-no-bind
  158:11  error  JSX props should not use arrow functions  react/jsx-no-bind

[mbrookes]

@newoga out of interest, what would you use instead of:

onTouchTap={() => this.props.onSelectAffix('pm')}

[newoga]

@mbrookes This is less of a style preference and more for performance! I didn't realize it until just recently when I read a blog post. I'm not quite the best approach of how to handle this all the time, but this would be better I think:

handleOnTouchTap(value) {
  this.props.onSelectAffix(value);
}

<Component prop={this.handleOntouchTap('pm')} />

[newoga]

Note, our performance issues related in the <DropDownMenu /> component might be related to this jsx-bind. (#2787). Our previous implementation in 0.13.x didn't using jsx binding but out current one in 0.14.x uses it on looped <MenuItem />. It'd be interesting to see if that fixes it.

[newoga]

The jsx-no-bind doc says this:

A bind call or arrow function in a JSX prop will create a brand new function on every single render. This is bad for performance, as it will result in the garbage collector being invoked way more than is necessary.

[oliviertassinari]

jsx-no-bind is definitely a performance linting rule. I was considering it on my project some time ago.
The thing is, it was too much code refactorization. In order to work. You almost always have to create a new component.
But now, we have stateless functional component. That point shouldn't be an issue anymore.
I think that it's a good idea to enforce this rule 👍.

but this would be better I think:

@newoga I think that your proposal doesn't solve anything 😁.
This should be better:

handleOnTouchTap(value) {
  this.props.onSelectAffix(value);
}

<Component prop={this.handleOntouchTap} type="pm" />

[newoga]

Haha, I knew something didn't look right when I typed it 😁! I guess the component that is calling the handleOnTouchTap would just have to pass type=pm back.

[newoga]

@oliviertassinari Maybe for DropDownMenu we should make MenuItem also pass the key prop back if it was provided. That way we could get rid of the .bind() in DropDownMenu . In other words, we already have the new new component, we're just not passing back the data we need.

[mbrookes]

@oliviertassinari No, this should be better. 😏 :

handleOnTouchTap() {
  this.props.onSelectAffix();
}

<Component prop={this.handleOntouchTap} type=“pm" />

@newoga thanks for the explanation, I wasn't aware of the performance implication - I'll keep it in mind for my own code.

[alitaheri]

@oliviertassinari That will still create a new element object. I don't think jsx-no-bind really does solve any performance issues. the only think I can think of is pure-rendering. if the same function is always passed down then pure-rendering can be implemented, otherwise it won't work. but with memory allocation jsx-no-bind doesn't solve a thing in this one case (with pure functions it does, but not with ones requiring a closure as mentioned by @mbrookes ).

I think this low level of optimization should be avoided as it complicates the code. If we properly implement pure-render (Recompose pure or onlyUpdateForKeys) we really shouldn't worry about a bunch of functions being created and destroyed. But I like the no-bind with allow-arrow, it's a lot prettier style 😁

[alitaheri]

@mbrookes @oliviertassinari Nope:

<Component prop={this.props.onSelectAffix} type=“pm"/>

[oliviertassinari]

@alitaheri Good point. We save one function allocation but we then have the memory allocation required for a component. Not that great 😁. I'm wondering if we can solve the binding without introducing a new component in some cases.

[mbrookes]

@alitaheri - ah, you win! 😆

[alitaheri]

@oliviertassinari Functions are immutable. you can't change what they have in their closures. in some cases it's simply impossible. I'd say we really shouldn't worry about this. I mean if we memoize style objects all our problems will be solved. and these functions will go unnoticed for as long as javascript lives 😆 There are much bigger fish to fry than this. let's cook'em first and then if profilers brag about these function allocations we'll come back to this issue 😁

@mbrookes

[newoga]

We save one function allocation but we then have the memory allocation required for a component. Not that great 😁.

I don't think that's correct, because we're looping through menu items:

const menuItemElements = menuItems
      ? menuItems.map((item, idx) => (
        <MenuItem
          key={idx}
          primaryText={item[displayMember || 'text']}
          value={item[valueMember]}
          onTouchTap={this._onMenuItemTouchTap.bind(this, idx, item)}
        />
      ))
      : React.Children.map(children, child => {
        const clone = React.cloneElement(child, {
          onTouchTap: this._onMenuItemTouchTap.bind(this, index, child.props.value),
        }, child.props.children);
        index += 1;
        return clone;
      });

According to the docs, this would be a new function allocation per component that is rendered rather than just one for all?

[alitaheri]

@newoga There is nothing we can do about it in this case since it's placing idx inside a closure. there MUST be a new function allocated for each element, or a simulated closure by an HOC, which will then have to allocate an element object for it. It's like the pigeon hole principle. you can't defy the fact that you need SOMETHING to hold that idx inside of it. it being a function closure, a mutable object (EXTREMELY dangerous), another element from an HOC, etc... you need to allocate something to hold a reference to where idx is in the computer memory 😁

[newoga]

@alitaheri Okay fair enough! 😄

[newoga]

I just updated my eslint-airbnb configuration in my personal project today and they started enforcing this:

http://eslint.org/docs/rules/no-unneeded-ternary

That might be a good one for us to add to the list.

@alitaheri This one is for you. 😆

[oliviertassinari]

@newoga I'm loving it!

[alitaheri]

Disallow Nested Ternaries (no-nested-ternary)

No more monsters within monsters 😆

[mbrookes]

There's nothing inherently wrong with nested ternaries, as long as they're formatted correctly.

This:

let thing = foo ? bar
  : (baz === qux) ? quxx
    : foobar;

is more readable than both of the following, and more compact than the latter:

let thing = foo ? bar : baz === qux ? quxx : foobar;

let thing;

if (foo) {
  thing = bar;
} else if (baz === qux) {
  thing = quxx;
} else {
  thing = foobar;
}

[Kosta-Github]

If you format your last example like this:

let thing;
if      (foo)         { thing = bar; }
else if (baz === qux) { thing = quxx; }
else                  { thing = foobar; }

[alitaheri]

There is indeed nothing wrong with well formatted nested ternary. I use it myself, to get away with singular expression:

const foo = cond1
  ? value1
  : cond2
    ? value2
    : value3;

The real issue is when value is expression itself...

const foo = criteria1 === stuff < 0
  ? [(stuff + bar), ...restOfTheStuff]
  : cond2
    ? [someOtherIrrelevantStuff]
    : {object: "?!!!"};

that begins to lose it's readability really fast. I have nothing against ternary, I even sometimes nest them 3 levels deep and it still remains readable.

But they should be used to make code easier to read, as should everything else! not make it harder. this applies to everything. even if/else chains. if it makes code readable use it, if it feels "clever", then it's wrong.

[oliviertassinari]

If you format your last example like this:

IMHO, using space to align the code is a bad idea for maintainability.

[mbrookes]

I think we all agree then - use appropriately, format properly, and !no-nested-ternary. :)

[mbrookes]

Would you consider adding this to the list? http://eslint.org/docs/rules/operator-linebreak

I noticed in Stepper that @namKolo put the + for string continuation linebreaks on the following line, which may be preferable, as extending a string onto a new line then only requires one line change, not two.

[oliviertassinari]

That's a good point. I think that the best to do is to use operator-linebreak: [2, "after"] so that, when reading a line, if there is an operator at the end, you know there is more.

[mbrookes]

Either works for me, but good to have the consistency.

[alitaheri]

I think we should add no-shadow to the list too.

[newoga]

I saw no-multiple-empty-lines in the reactjs eslint configuration and thought that could be good idea.

[mbrookes]

Lets hold these until we get the bulk of the bugfix / enhancement PRs merged.

[newoga]

Lets hold these until we get the bulk of the bugfix / enhancement PRs merged.

I agree, let's do a freeze on eslinting for now until we're ready to do a lot of the big remaining pieces. When that's done we'll give @oliviertassinari a few days to perfect the rest of these 😁

[oliviertassinari]

Alright, let's wait.
I'm gonna continue investigating react native (v20 is breaking it, dam it).

[nathanmarks]

@oliviertassinari

please please please react/no-multi-comp: 2 😄 The dialog.jsx file is driving me nuts.. lol