Add ESLint Rule for Safe Ref Usage

[louisscruz]

Summary

Mutating a given ref's current property as the result of a render or hook call is not safe in concurrent mode. The reason for this is described here.

As an example, this is not safe:

const MyComponent = React.memo({ someProp }) => {
  const myRef = useRef();

  myRef.current = someProp;

  // ...
});

This also wouldn't be safe:

const useMyCustomHook = () => {
  const [someState, setSomeState] = useState('');
  const myRef = useRef();

  myRef.current = someState;

  // ...
};

There currently isn't a guard rail in place to prevent applications not written for concurrent mode first to easily identify this problem before the application is opted-in to concurrent mode. Without a guard rail, applications that move from non-concurrent mode to concurrent mode are at some point are at a serious risk of experiencing cumbersome regressions as a result of the move.

So, I'm proposing an ESLint rule that fails in the case that any top-level ref mutations are found and suggests that they be moved into either useEffect or useLayoutEffect hook calls so that way the mutation only occurs in the case that a commit to render has happened.

At the time that this PR is opened, the rule is called safe-ref-mutations. It is initially not marked as recommended because I'd like to get some core React contributor guidance first.

This rule didn't seem to fit in with either rules-of-hooks or exhaustive-deps, so I wrote it as its own rule. If there's a preference to move it into one or the other, I'm happy to do so. But in general, I wonder if there are more rules that React should have in place for safe concurrency mode usage. Maybe some kind of safe-concurrency rule would be better?

As part of this PR, I needed some functions that accomplished the same as those in RulesOfHooks.js, so I've extracted them into a utils.js for re-use.

If there's a preference that I open an issue for this, I'm happy to do so.

Test Plan

The ESLint rule will be heavily tested if this is the route that is chosen.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e521c8a:

Sandbox	Source
weathered-butterfly-h2j8z	Configuration

[sizebot]

Details of bundled changes.

Comparing: 4c7036e...e521c8a

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+7.0%	+7.3%	82.28 KB	88.05 KB	18.81 KB	20.19 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+6.7%	🔺+6.2%	22.87 KB	24.4 KB	7.7 KB	8.17 KB	NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against e521c8a

[sizebot]

Details of bundled changes.

Comparing: 4c7036e...e521c8a

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+7.0%	+7.3%	82.29 KB	88.06 KB	18.82 KB	20.19 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+6.7%	🔺+6.2%	22.89 KB	24.41 KB	7.71 KB	8.18 KB	NODE_PROD

Size changes (experimental)

Generated by 🚫 dangerJS against e521c8a

[EECOLOR]

@louisscruz You point at a comment talking about a very specific use case where a ref is used inside a callback that is returned from a hook. This allows that code to be used anywhere in the render tree.

With the examples you provide, you simply state: 'this is not safe'. Could you explain how these constructs could cause problems?

[louisscruz]

@EECOLOR #14099 (comment)

If one branch’s rendering is interrupted, but its rendering has the side effect of mutating a ref, that ref value will still have been mutated and retain the value intended for that branch rendering in other branch renderings.

[EECOLOR]

@louisscruz Thank you for responding. It however does not help me understanding the problematic nature. How can we achieve the same instance of a component to end up in two branches?

[stale]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[stale]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!