Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no-arrow-condition does not recognise legitimate func #4417

Closed
nkbt opened this Issue Nov 14, 2015 · 17 comments

Comments

Projects
None yet
6 participants
@nkbt
Copy link

nkbt commented Nov 14, 2015

20151114-213400

Code from https://github.com/nkbt/react-collapse/blob/master/src/Collapse.js#L45-L53:

    return (
      <Motion
        defaultStyle={{height: 0}}
        style={{height: spring(isOpened ? fixedHeight : 0, springConfig)}}>
        {({height}) => (!isOpened && parseFloat(height).toFixed(1) === '0.0') ? null : (
          <div style={{...style, height, overflow: 'hidden'}} {...props}>
            {children}
          </div>
        )}
      </Motion>
    );
@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Nov 14, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Nov 14, 2015

@nkbt

This comment has been minimized.

Copy link
Author

nkbt commented Nov 14, 2015

  1. The version of ESLint you are using (run eslint -v)
    1.9.0

  2. What you did (the source code and ESLint configuration)

    {({height}) => (!isOpened && parseFloat(height).toFixed(1) === '0.0') ? null : <div />}

    .eslintrc

    "parser": "babel-eslint",
    
    // ...
    "no-arrow-condition": 2, // disallow arrow functions where a condition is expected
  3. The actual ESLint output complete with numbers

    /Users/nkbt/nkbt/react-collapse/src/Collapse.js
     48:10  error  Arrow function used ambiguously with a conditional expression  no-arrow-condition
    
  4. What you expected to happen instead
    No errors, it is a function, not a condition

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 14, 2015

This is expected behavior. Your code is the equivalent of this:

var fn = ({height}) => (!isOpened && parseFloat(height).toFixed(1) === '0.0');

// later
{ fn ? null : <div />}

You are testing the truthiness of a function, which is always true.

@nzakas nzakas closed this Nov 14, 2015

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 14, 2015

@nzakas That's wrong, your parser has a precedence bug. Conditional expressions allow logical or expressions as the test, not assignment expressions.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 14, 2015

no-arrow-condition seems to report all arrow functions which has ConditionalExpression in its body.
I felt this behavior is noisy.

https://github.com/eslint/eslint/blob/master/lib/rules/no-arrow-condition.js#L73

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 14, 2015

Oh, wow. Yeah there doesn't seem to be any reason for that check.

@nkbt

This comment has been minimized.

Copy link
Author

nkbt commented Nov 14, 2015

@nzakas arrow func in this case is confused with a check in condition. It is not a check, it is a callback function with arguments destructuring.

{({height}) => (!isOpened && parseFloat(height).toFixed(1) === '0.0') ?
  null :
  <div style={{...style, height, overflow: 'hidden'}} {...props}>{children}</div>}

which is effectively

  function render (props) {
    var height = props.height;
    if (!isOpened && parseFloat(height).toFixed(1) === '0.0') {
      return null;
    } else {
      return <div style={Object.assign({}, style, {height: height, overflow: 'hidden'}) {...props}>{children}</div>;
    }
  }
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 15, 2015

@michaelficarra ah! I see, thanks for explaining. This is a big in Espree then.

@nzakas nzakas reopened this Nov 15, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 15, 2015

Oops, just saw @mysticatea's comment, so that's an easier fix.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Nov 17, 2015

I thought we can just deprecate no-arrow-condition rule, because no-constant-condition rule is covering all of no-arrow-condition rule.
ArrowFunctionExpression is a constant, and no-constant-condition is reporting that.

image

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 17, 2015

Interesting. no-constant-condition warns about a lot more things, though. Will people want all the warnings or just arrow functions?

@lukekarrys

This comment has been minimized.

Copy link
Contributor

lukekarrys commented Nov 17, 2015

Oh, wow. Yeah there doesn't seem to be any reason for that check.

When I ported no-arrow-condition over from a plugin, there was some previous discussion about this, so I wanted to add some background as to why legitimate arrow functions are being flagged.

The original plugin would always flag statements like the following because it could be a mistake so the author should instead be explicit and rewrite the arrow function with an explicit return statement.

/*eslint-env es6*/
/*eslint no-arrow-condition: 2*/

var fn = a => b ? c : d
// 4:10 - Arrow function used ambiguously with a conditional expression. (no-arrow-condition)
// To fix this write you arrow function like this
var fn = a => { return b ? c : d }

There was discussion in the PR about adding an option to allow for conditions in arrow functions and only flag cases where it was a more obvious mistake (like using an arrow function inside an if statement), but I think it was decided to wait for feedback once it went live

Should an option on no-arrow-condition be added now, or will the rule be deprecated instead for no-constant-condition?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 17, 2015

We don't like having overlapping rules, so we need to make some kind of change. I'm wondering if we should have something like no-confusing-arrows that can catch strange cases like this anyway.

@lukekarrys

This comment has been minimized.

Copy link
Contributor

lukekarrys commented Nov 17, 2015

I'm wondering if we should have something like no-confusing-arrows that can catch strange cases like this anyway.

I think that's a good idea. That rule would catch these cases and there'd be no need for an option since that's all the rule would cover. Then no-constant-condition would cover the rest, and no-arrow-condition could be deprecated.

@nzakas nzakas added rule breaking and removed bug labels Nov 18, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 18, 2015

Cool, let's do that. We will deprecate no-arrow-condition in favor of no-constant-condition, and add no-confusing-arrows to cover the rest.

@lukekarrys

This comment has been minimized.

Copy link
Contributor

lukekarrys commented Nov 18, 2015

Sounds good, I can work on that.

Should I split it up into two PRs (one to create no-confusing-arrows and one to add a deprecation notice to no-arrow-condition)?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 19, 2015

Yes, please.

@nzakas nzakas closed this in #4687 Dec 13, 2015

nzakas added a commit that referenced this issue Dec 13, 2015

Merge pull request #4687 from lukekarrys/issue4417
New: Add no-confusing-arrow rule (ref #4417)

nzakas added a commit that referenced this issue Dec 14, 2015

Merge pull request #4688 from lukekarrys/issue4417-2
Breaking: deprecate `no-arrow-condition` rule (fixes #4417)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.