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 need to require handler names to be scoped to an object #2470

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

aub
Copy link
Contributor

@aub aub commented Oct 21, 2019

In the jsx-handler-names rule, the code requires that the handler being checked is scoped to an object. This means that it will check onChange={this.wackyHandler} but because it requires the scoping it won't attempt to check handlers like onChange={wackyHandler}. In React.FC components we use this type of handlers all the time, since you can pass a locally-defined function as the handler. This change just removes a test that was causing it to return without the object scoping and then adds tests for a few conditions that it now covers.

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.

The rule is not intended to force the name of local variables - only prop names and class component method names.

Even if this was a desired change, it would have to be behind an option.

@aub
Copy link
Contributor Author

aub commented Oct 22, 2019

Thanks for the feedback! I did add a 'checkLocalVariables' option to the rule and a few more tests to check it with various combinations options. It seems to be like it's valuable to make sure that local variables follow the same naming conventions for handlers as ones on an object, and this is something we could use in our own code, specifically for the functional component situation I mentioned before.

@ljharb
Copy link
Member

ljharb commented Oct 24, 2019

@aub hm, unfortunately since you made a PR from a fork that (i assume) you don't have explicit write access on, but instead have implicit access via org ownership or similar, I'm unable to force push to your PR branch (this should work, but since virtually everybody makes PRs from their personal fork, it hadn't cropped up as a problem until recently). Could you rebase this down to one commit with the commit message [New] `jsx-handler-names`: add `checkLocalVariables` option, and force push?

@aub
Copy link
Contributor Author

aub commented Oct 25, 2019

Closing this in favor of #2474, which is the same change but submitted from a different fork and under a single commit.

@aub aub closed this Oct 25, 2019
@ljharb
Copy link
Member

ljharb commented Oct 25, 2019

@aub Is there any way you could reopen this and match the sha of the other one?

Opening a second PR permanently pollutes the git graph :-/

@aub aub reopened this Oct 29, 2019
@aub aub force-pushed the fix-object-scoping-for-handlers branch from 0071482 to 8093565 Compare October 29, 2019 16:29
@aub
Copy link
Contributor Author

aub commented Oct 29, 2019

Ok, I've closed the second PR and I believe I have the commit the way you wanted it.

@ljharb ljharb merged commit 8093565 into jsx-eslint:master Oct 29, 2019
@ljharb
Copy link
Member

ljharb commented Oct 29, 2019

Thanks!

@jsphstls
Copy link
Contributor

I was dealing with the same issue that prompted this PR. I no longer use class components and props are often destructured. Without this flag, the rule really does not work. With the flag, I see false positives where handler props are passed through.

const SomeButton = ({ onClick }) => <button onClick={onClick} />

@aub do you have any tips for how to use this flag?

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

Successfully merging this pull request may close these issues.

3 participants