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

jsx-uses-vars breaks eslint prefer-const #716

Closed
lukeapage opened this issue Jul 26, 2016 · 10 comments
Closed

jsx-uses-vars breaks eslint prefer-const #716

lukeapage opened this issue Jul 26, 2016 · 10 comments

Comments

@lukeapage
Copy link
Contributor

lukeapage commented Jul 26, 2016

I have code like this

function X(props) {
    const { filterList } = props;

    let filters = _.chain(filterList)
        .map((filter, index) =>
            <Z/>
        )
        .value();

    return (
        <div>
            {filters}
        </div>
    );
}

but with prefer-const on, the prefer const error on filters is removed when I switch on jsx-uses-vars

@lukeapage
Copy link
Contributor Author

@yannickcr
Copy link
Member

Thanks for the issue, but if I'm right there is nothing we can do on our side to prevent this, since the issue seems to come from ESLint itself (eslint/eslint#5837), right?

@ljharb
Copy link
Member

ljharb commented Jul 27, 2016

It sounds like with prefer-const there's a (correct) error, and with prefer-const + jsx-uses-vars, there's (incorrectly) not an error.

@yannickcr
Copy link
Member

prefer-const is ignoring variables with eslintUsed set on it. Since that's how we mark a variable as used in ESLint I don't see what we can do here 😐

But:

  • We mark JSXOpeningElement (<App />) and JSXExpressionContainer ({filters}) as used but ESLint seems to correctly handle JSXExpressionContainer without the help of this rule. Maybe we could remove this? (All tests are still passing without it, but maybe we're missing a pattern?)

This will solve the case described by @lukeapage but we'll still have the problem with JSXOpeningElement:

let App = <div />; // should warn "'App' is never reassigned. Use 'const' instead" but will not
<App />

@lukeapage
Copy link
Contributor Author

Yeah sorry for not updating after tracking down.

I left this open until a solution is found, in case something needs to be done here as welll.

@Wilfred
Copy link
Contributor

Wilfred commented Aug 23, 2016

I hit this today. A minimal repro:

function foo() {
  let iCouldBeConst = 6;
  return <th colSpan={iCouldBeConst}></th>;
}

and eslint config:

{
    "parserOptions": {
        "ecmaVersion": 6,
        "ecmaFeatures": {
            "jsx": true
        },
        "sourceType": "module"
    },
    "rules": {
        "react/jsx-uses-vars": 2,
        "prefer-const": 2
    },
    "plugins": [
        "react"
    ]
}

Sounds like removing eslintUsed on JSXExpressionContainer would be a definite improvement, since that would fix both my situation and OP.

@yannickcr
Copy link
Member

Sounds like removing eslintUsed on JSXExpressionContainer would be a definite improvement, since that would fix both my situation and OP.

Yes, I will remove this. It does not fix the whole issue with prefer-const but it's better than nothing for now.

@yannickcr
Copy link
Member

Keep it open for now due to #716 (comment)

@yannickcr yannickcr reopened this Aug 23, 2016
@Wilfred
Copy link
Contributor

Wilfred commented Aug 23, 2016

Thanks :)

@yannickcr
Copy link
Member

Fixed in ESLint 3.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants