eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

Feature request: ban createRef in function components

Open danvk opened this issue 3 years ago • 12 comments

Using createRef in a function component is almost always a mistake because it will create a new ref on every render. Usually you want useRef (see this discussion). Since this is such an easy mistake to make, it would be helpful to have a linter rule to flag it.

danvk avatar Apr 13 '21 13:04 danvk

That seems like a good rule for eslint-plugin-react-hooks. This plugin can’t assume you have hooks available, so such a rule wouldn’t be useful for react versions without them.

ljharb avatar Apr 13 '21 15:04 ljharb

Thanks @ljharb. I filed an issue against the React repo, we'll see what they say: https://github.com/facebook/react/issues/21256

danvk avatar Apr 13 '21 21:04 danvk

IMO this does seem like something that should be warned about, but it also seems odd for eslint-plugin-react-hooks to warn about it (since those rules are all about hooks and createRef is not a hook).

Regardless of whether the version of React in question supports hooks, using createRef in a function body like this is always a mistake, right?

bvaughn avatar Apr 13 '21 23:04 bvaughn

Not necessarily - it could be memoized manually, or used inside another memoizing hook.

ljharb avatar Apr 13 '21 23:04 ljharb

Not when it's in the top level body of the function component (not within a conditional) as shown in the linked example.

or used inside another memoizing hook.

Also, could you share the example you have in mind of when it would be appropriate for a custom hook to use createRef rather than useRef for memoization? :)

bvaughn avatar Apr 13 '21 23:04 bvaughn

(warning: off the top of my head) Something like:

function Foo({ a }) {
  const ref = useMemo(() => createRef(), [a]);
  const fn = useCallback(() => doSomethingWith(ref?.current), [ref]);
  return <Bar onSomething={fn} />;
}

Obviously you might prefer useRef here instead of useMemo - but that doesn't mean there's something wrong with this usage.

ljharb avatar Apr 15 '21 23:04 ljharb

Obviously you might prefer useRef here instead of useMemo - but that doesn't mean there's something wrong with this usage.

Kind of does 😄 Seems to suggest (to me) some fundamental misunderstanding of how the APIs work.

For example, recreating the ref object itself when a dependency changed seems to defeat the purpose of a ref (as a stable, mutable object) in the first place? But also, using a built-in hook and a built-in API to do the same thing as a built-in hook seems to suggest you aren't aware of the built-in hook (and a lint rule telling you about it seems fine).

bvaughn avatar Apr 15 '21 23:04 bvaughn

@ljharb that usage of createRef seems fine and @bvaughn's caveat about "in the top level body of the function component" would prevent a false positive on it.

danvk avatar Apr 15 '21 23:04 danvk

That’s all a reasonable position to take.

Unfortunately, eslint-plugin-react-hooks doesn’t export (or depend on a package for) the hook detection logic, which means we have to copy-paste it, which seems very unfortunate.

@bvaughn even though i really do think any rule dealing with hooks whatsoever belongs in the “react hooks” eslint plugin (that’s the implied position you occupied by publishing a so-named eslint plugin, imo), if you think it would be better in this plugin, then could at least the hook logic be factored out so we can reuse it?

ljharb avatar Apr 16 '21 00:04 ljharb

cc @gaearon who may have thoughts here. (He's the primary owner of the hooks lint rule.)

bvaughn avatar Apr 16 '21 00:04 bvaughn

While the proper home for this rule up in the air, my workaround is to blanket ban createRef using the standard no-restricted-imports and no-restricted-properties rules and use eslint-disable to whitelist existing class components.

  {
    'no-restricted-imports': [
      'error',
      {
        paths: [
          {
            name: 'react',
            importNames: ['createRef'],
            message: 'You probably want React.useRef instead.'
          }
        ],
      },
    ],
    'no-restricted-properties': [
      'error',
      {
        object: 'React',
        property: 'createRef',
        message: 'You probably want React.useRef instead.'
      }
    ]
  }

danvk avatar Nov 16 '21 15:11 danvk

I've resorted to adding yet another plug-in, https://github.com/rel1cx/eslint-react, just so that I can enable its no-create-ref rule.

eirikur-grid avatar Apr 04 '24 15:04 eirikur-grid