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-no-bind] Only warn for props and account for variable declaration #1459

Merged

Conversation

jackyho112
Copy link
Contributor

@jackyho112 jackyho112 commented Oct 2, 2017

This will fix #1444, #1395 and #1417, so that the rule will only warn if there are violations in JSX attributes and will keep track of const variable declarations of violations.

For examples,

renderButton(someParams){
  const clickHandler = this.handleClick.bind(this, someParams);
  return(
    <ButtonComponent
      onClick={clickHandler}
    >
      Submit
    </ButtonComponent>  
  )
} 

will warn.

But

 render() {
    return (
      <div>
        {this.props.list.map(this.wrap.bind(this, 'span'))}
      </div>
    );
  }

will not.

The new implementation is simply keeping track of each const variable declaration that references a violation in each block statement and reports them if they got passed into JSX props.

With the update, this rule does not account for any assignment expression and variable declarations that are not const. I feel that it is acceptable to leave them for future updates.

Please let me know if there is any idea for improvements. 😄

Fixes #1444. Fixes #1395. Fixes #1417.

@jackyho112 jackyho112 changed the title [jsx-no-bind] Make the rule only warn for violation in props [WIP][jsx-no-bind] Make the rule only warn for violation in props Oct 14, 2017
@jackyho112 jackyho112 changed the title [WIP][jsx-no-bind] Make the rule only warn for violation in props [jsx-no-bind] Only warn for props and account for variable declaration Oct 15, 2017
) {
return;
}
const ancestors = context.getAncestors(callee).reverse();
Copy link
Contributor Author

@jackyho112 jackyho112 Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this block of code reports any violation in a component's render method. This rule is supposed to only report violations in JSX props, so this is not needed.

' }',
'};'
].join('\n'),
parser: 'babel-eslint'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of these test cases don't need babel-eslint; can we use the default parser wherever possible?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about async arrow functions?

@jackyho112
Copy link
Contributor Author

@ljharb Forgot about them. Will fix. 😅

@jackyho112
Copy link
Contributor Author

jackyho112 commented Oct 25, 2017

@ljharb

Thanks for the feedback! I have gotten rid of all the unnecessary babel-eslint in the tests. For async arrow functions, I think the code has already accounted for it by checking all the block statements, so I just added more tests regarding that.

Now I will always keep async arrow functions in mind for future contributions. let me know if there is anything else I can improve on this PR. 😄

@ljharb
Copy link
Member

ljharb commented Oct 25, 2017

@jackyho112 I was mainly asking about ensuring that <Foo prop={async () => {}} /> warns too :-)

@jackyho112
Copy link
Contributor Author

@ljharb 😄 , didn't know. I'm on it!

@ljharb
Copy link
Member

ljharb commented Oct 25, 2017

Let's also ensure that function () {}, function * () {}, and async function () {} all warn as well :-)

@jackyho112
Copy link
Contributor Author

@ljharb Good call. Gonna have to do it tomorrow though.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this LGTM

@jackyho112
Copy link
Contributor Author

Thanks for the approval!

@ljharb ljharb requested review from yannickcr and lencioni October 27, 2017 01:10
@ljharb ljharb requested a review from EvHaus October 27, 2017 01:10
@lencioni lencioni merged commit fcf580b into jsx-eslint:master Oct 30, 2017
@jackyho112 jackyho112 deleted the make-jsx-no-bind-only-warn-for-props branch October 30, 2017 18:59
@toearth
Copy link

toearth commented Dec 20, 2017

537b6fdc-f92b-4cdc-b55e-09271415c8f1
However, with react-navigation options, it can not pass the rule

@ljharb
Copy link
Member

ljharb commented Dec 20, 2017

@windhost can you file a separate issue for that? since NavigationOptions returns an object, it shouldn't be treated like a component.

@toearth
Copy link

toearth commented Mar 22, 2018

@ljharb 👌

@toearth
Copy link

toearth commented Mar 22, 2018

#1736

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants