-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New jsx-no-ref rule #1060
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
base: master
Are you sure you want to change the base?
New jsx-no-ref rule #1060
Conversation
Love this, @prometheas! ❤️ I'll definitely be recommending this to everyone. |
Won’t this become the new “setState is bad”? There are tricks with passthrough ref props (e.g. |
I don't think so. Putting As for "setState is bad", how did people ever even start down that path? ;) |
Why is it OK to add a ref on DOM elements? The examples seem to imply that string refs are ok and callback refs are not. Could you make sure the examples for both valid and invalid contain inline callback refs as well as string refs? |
How do you solve cases like needing to focus a component that manages text input after a click on its sibling? Or select all text in it. |
Also included related and unrelated changes in the rule’s documentation markdown file.
|
||
## When not to use | ||
|
||
If you are not using JSX then you can disable this rule. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you absolutely think this rule is necessary then it would be good to document good use cases for refs, for example triggering focus, selection, and imperative animations. These are important use cases and can be hard with React declarative model.
One point I think is worth considering: given that ESLint rules are intended to be ignored, warned about, or report errors by means of In that light, it's worth considering whether part of the feedback might be best considered in the frame of default error levels (perhaps it defaults to ignore or merely warn). |
react-virtualized uses this type of I don't think it's a good idea to treat it as always-bad. |
OK, I shouldn't have said it's always a bad idea. I haven't ever needed a ref on a custom component, but apparently you have 😅 I'm not suggesting we put this in the recommended set. But I still believe this rule could go a long way toward helping people write better code, which is partly what a linter is supposed to do. |
Sure. I agree with that. We should aim to communicate when it's appropriate though so people don't make tl;dr assumptions and miss out on a useful tool (albeit with limited application). |
Sounds like something that ought to be added to the markdown file. I'm unfortunately quite new to React, however, so I'm not sure I can really nail a solid summary on this one. Does anyone have some advice for me on language to use for an edit? |
@prometheas This recent addition to the React docs may be helpful: https://facebook.github.io/react/docs/refs-and-the-dom.html#when-to-use-refs |
Thanks, @bvaughn; on it! |
@prometheas given the updated react docs on refs, do you still think this rule is useful? if so, could you rebase it and mark it as ready for review? |
Oh my goodness, echoes from days of yore 🤣 In the 4 years since I had originally contributed this PR, the only real "must-use" scenario I've frankly encountered (and, for the record, it's hard to describe it as "rarely") is the Still, I definitely also see plenty of scenarios in which the Let me revisit this PR sometime in the days ahead, as well as get my head around what may have changed in the docs. |
Implemented rule that flags use of
ref
attribute in user-defined JSX tags, as this is considered a smell of non-declarative component design.cc: @mjackson