[Fix] sets default for jsx-no-bind.ignoreRefs to true

[dimitropoulos]

TL;DR

Refs are special. Inline arrow functions in refs do not cause re-renders like they do for props.

Context

The jsx-no-bind rule exists to prevent users from a common (and easy to make) mistake where unnecessary renders occur. This makes sense for props, but there are two special cases to this, ref, and key. For the longest time both of these were value types (strings), but later on ref patterns emerged that used callbacks.

Problem

I have seen many codebases that do things like:

  setRef(container: HTMLDivElement | null) {
    this.container = container;
  }

in react component classes out of a misunderstanding and an over-generalization of the jsx-no-bind rule.

Demo

I prepared a demo that demonstrates what I'm talking about, https://codesandbox.io/s/inline-arrow-refs-do-not-cause-rerenders-4cdmf?file=/src/App.js.

Solution

This PR switches the default of ignoreRefs from false to true.

One step further

I believe that it's not a matter of preference, and that even encouraging users to do setRef above is a clear antipattern. I believe it is an antipattern because it reinforces a misunderstanding about how refs actually work in react. Despite that, I'm trying to be considerate to people that like things how they like things. Due to that, instead of removing ignoreRefs altogether (and hardcoding the behavior to what true functions as today), I've oped to make a PR that just switches the default.

[codecov-commenter]

Codecov Report

Merging #2991 (98be338) into master (9aa539d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2991   +/-   ##
=======================================
  Coverage   97.59%   97.59%           
=======================================
  Files         110      110           
  Lines        7268     7269    +1     
  Branches     2651     2650    -1     
=======================================
+ Hits         7093     7094    +1     
  Misses        175      175           

Impacted Files	Coverage Δ
lib/rules/jsx-no-bind.js	100.00% <100.00%> (ø)
lib/rules/jsx-child-element-spacing.js	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9aa539d...98be338. Read the comment docs.

[ljharb]

That very example you created actually demonstrates why it's so critical to avoid inline ref callbacks. When I click "switch Child to inline arrow ref", and click "force Parent to render" multiple times, "calling inline arrow ref" is logged with a ref of null and then again with a ref of Child, every time.

When I switch it to a bound ref, and click "force Parent to render" multiple times, setChildRef is only ever called once with null and once with Child.

In other words, the antipattern is objectively and decidedly to use an inline arrow ref callback.

[dimitropoulos]

I believe you are mistaken about the consequences of function being called twice - as always, I'm open to being mistaken, myself.

To quote from the rule (emphasis mine):

This is bad for performance, as it may cause unnecessary re-renders if a brand new function is passed as a prop to a component that uses reference equality check on the prop to determine if it should update.

Note the part about unnecessary re-renders. Refs are crucially different from props in this way. I have seen this as a common point of confusion among react developers.

Refs do not cause re-renders when you pass a new function. This is exactly what my example demonstrates.

It's commonly confused by people, in my experience, in part because with a function component it has a special behavior (compared to regular props, with respect to refs). To quote from the docs (https://reactjs.org/docs/refs-and-the-dom.html#caveats-with-callback-refs)

If the ref callback is defined as an inline function, it will get called twice during updates, first with null and then again with the DOM element. This is because a new instance of the function is created with each render, so React needs to clear the old ref and set up the new one. You can avoid this by defining the ref callback as a bound method on the class, but note that it shouldn’t matter in most cases

Note, the "this" they are talking about avoiding is not re-renders. It's simply calling the function twice because it's resetting it, as the note describes.

---

If I missed some context or was wrong about something, could you please say why "it's so critical to avoid inline ref callbacks", since they don't cause the behavior that this rule is trying to ward against (re-renders)?

[ljharb]

I agree it does not cause rerenders.

However, unnecessarily calling the ref callback is a huge problem, even if it causes no rerenders. Perhaps the docs need updating, but the defaults do not. The rule is about general performance problems - not just avoiding rerenders.

[dimitropoulos]

Thanks for clarifying. I'm sorry to be a bother but I'd really like to understand why you believe the ref callback being called twice (to reset) is a huge problem (logically), or even a performance problem. The author's of the react docs, at least, say it is likely not to matter, and that's been my experience as well.

Can you point me in the direction of some way I can measure or otherwise observe this performance problem you're referring to?

And, to clarify, it's not calling the callback unnecessarily, the necessity of calling back is described in the docs I quoted above. The demo was not constructed to focus on this, but rather re-renders. The resetting call can be avoided with useCallback (or useRef or createRef) but I didn't add that because it wasn't the point of the demo.

Thanks

[ljharb]

Indeed - using one of those hooks-based approaches is fine as well, because the same === function gets passed as the ref.

It's an issue because a) work is sometimes done in these ref callbacks, and repeating that work is expensive; and b) in my experience it's exceedingly likely that developers will not expect a ref callback to be called with null after it's called with an element, which can cause a lot of bugs when the callback is called multiple times.;

[dimitropoulos]

It sounds like, then, closing this PR and updating the documentation is the best way forward.

I opened #2998 to update the docs.

For what it's worth I don't agree at all this is a "critical" performance problem. The thing you are concerned about sounds more like an architectural problem to me. However, I'm not surprised by the React behavior, having read the docs, and I understand what you're saying that someone that hasn't read and digested every word of the docs may be surprised by this. To put it another way, if your ref setter method, of all things, does that much work that it being called is a worry, that to me seems like a different problem/concern altogether.