Recoil: Add lint rule for useRecoilCallback deps array

[itsMapleLeaf]

Custom hooks that have custom dependency arrays can't be statically analyzed by the react-hooks linter rule, and can't have their dependencies automatically tracked. Because of this, I'd suggest either:

1. Accept a callback and add that callback to the deps array of the internal useCallback. It'll make the callback on each render, or if the passed callback is useCallback'd, whenever the passed callback is recreated

2. Use the 'effect ref' pattern, so the end user can always pass a flat callback, but the callback function doesn't have to be a part of the deps array:

   function useRecoilCallback(fn) {
     const fnRef = useRef(fn)
     useEffect(() => {
       fnRef.current = fn
     })
   
     return useCallback(() => {
       // read fnRef.current instead of fn
     }, [some, deps])
   }
   

I think 1. is "more correct" and would result in less bugs both internally and for the end user, but 2. would result in better UX, since 1. would require the user to useCallback/useMemo themselves if they wanted a properly recreated callback. A note in the docs about that might help, but those are fairly easy to miss 😅

Either way, I think this change will give a better UX when working with the hooks lint rule, with less surprises in general

[drarmstr]

A design goal is to try to keep the API consistent with React. We debated this internally between requiring the user to know they have to nest useCallback or considering useRecoilCallback as a hook to work with callbacks that also provides access to Recoil state, and thus being consistent with the React hooks style of memoization. I think a fix for this approach is to update/provide a lint rule to also handle the deps array. However, the "effect ref" pattern looks interesting. It isn't fully consistent, but seems easier at first blush. Can we think of any downsides to essentially always having a consistent callback function per store?

[acutmore]

When this issue was brought up the React team said:

Our general guidance is that custom Hooks should prefer not to have a deps argument
https://github.com/facebook/react/issues/14920#issuecomment-470325790

I admit that custom hooks look much nicer with their own deps array, but the lost of the eslint rule is significant. I find it catches things I'm missing multiple times a day.

// clean but risky
const getValue = useRecoilCallback(({get}) => {
  ... // there is code here
}, [dep1, dep2]);

// without deps array. An eye full
const getValue = useRecoilCallback(useCallback(({get}) => {
  ... // there is code here
}, [dep1, dep2]));

// slightly shorter name hook perhaps?
const getValue = useRecoil(useCallback(({get}) => {
  ... // there is code here
}, [dep1, dep2]));

Option 2 of the 'effect ref' pattern. I believe it's safe as long as the callback is only called after commit. i.e. not during render or useLayoutEffect.

EDIT: +1 for a recoil lint rule. Wish I knew why the react lint rule doesn't allow the names of customHooks to be added as config. Would solve this for so many libraries. Must be a good reason.

[drarmstr]

Option 2 defaults to the passed in fn, though, so shouldn't it work during render?

[itsMapleLeaf]

I think he's referring to a case like this:

useEffect(() => {
  fn()
}, [])

const fn = useRecoilCallback(() => {})

Where the callback argument passed to useRecoilCallback _might_ be stale at the time of calling it, since the first effect here will run before the one that sets the callback to the ref.

I think this is a pretty tight edge case, though. I've used this pattern a lot, and I haven't run into a case where this was a problem (that I know of)

[acutmore]

Option 2 defaults to the passed in fn, though, so shouldn't it work during render?

On the first render it will be okay, but on subsequent renders any values that the function closes over will be stale.

I think this is a pretty tight edge case, though. I've used this pattern a lot, and I haven't run into a case where this was a problem (that I know of)

Very true, I've also used the pattern without issue. Though I do think there is a difference between using the pattern in an app which can have product specific tests to catch issues. Compared to using the pattern in a library that might be used by thousands of different apps, where edges cases now become statistically more common.

The react docs do literally say that they don't recommend this pattern. My guess is that it isn't safe with future changes to React (concurrent mode).

In either case, we don’t recommend this pattern and only show it here for completeness.

https://reactjs.org/docs/hooks-faq.html#how-to-read-an-often-changing-value-from-usecallback

[drarmstr]

Yeah, will pursue the lint rule.

[itsMapleLeaf]

It actually looks like the default lint rule may be getting customization soon? https://github.com/facebook/react/pull/18861

[acutmore]

It looks like customisation is already available:

https://github.com/facebook/react/blob/9752d31f127037e8126177b0456ab1b0547eb2db/packages/eslint-plugin-react-hooks/README.md#advanced-configuration

// .eslint.js
module.exports = {
  rules: {
    // ...
    "react-hooks/exhaustive-deps": [
      "warn",
      {
        additionalHooks: "useRecoilCallback",
      },
    ],
  },
};

A downside is that each project will separately need to remember to add this. Having this packaged up so it's a one line recommended eslint plugin might be less typo prone.

[jaredpalmer]

@acutmore we should just add this to getting started/installation instructions. seems totally reasonable.

[acutmore]

Great idea @jaredpalmer. I've raised PR #203 as a starting point.