From 5a75ce186a43a94708b9c4f89a42971e9a073e18 Mon Sep 17 00:00:00 2001
From: Gokhan Kurt <krtgkn@gmail.com>
Date: Thu, 9 Sep 2021 00:14:02 +0300
Subject: [PATCH 1/3] RFC: Callback Ref Cleanup

---
 text/0000-callback-ref-cleanup.md | 199 ++++++++++++++++++++++++++++++
 1 file changed, 199 insertions(+)
 create mode 100644 text/0000-callback-ref-cleanup.md

diff --git a/text/0000-callback-ref-cleanup.md b/text/0000-callback-ref-cleanup.md
new file mode 100644
index 00000000..80f9c25b
--- /dev/null
+++ b/text/0000-callback-ref-cleanup.md
@@ -0,0 +1,199 @@
+- Start Date: 2021-09-08
+- RFC PR: (leave this empty)
+- React Issue: (leave this empty)
+
+# Summary
+
+Callback Refs should allow returning a cleanup function which will be called when the ref is removed or changed.
+
+# Basic example
+
+```jsx
+function MyComponent(props) {
+  const logClicks = useCallback((node) => {
+    if (!node) return;
+
+    const onClick = () => console.log('clicked!');
+
+    node.addEventListener('click', onClick);
+
+    return () => {
+      node.removeEventListener('click', onClick);
+    };
+  }, [])
+
+  return <button ref={logClicks} />;
+}
+```
+
+# Motivation
+
+Similar to `useEffect`, Callback Refs often create resources that need cleaning up
+when an element is unmounted. In such situations, developers often keep a reference to
+the element in a mutable ref, and clean it up the next time Callback Ref is 
+called with a `null` (See section `Alternatives`). As in the basic example above, 
+it is easy to clean up the effects of a Callback Ref  when the element is already in the 
+scope of the cleanup function.
+
+This feature also opens the way for two useful code patterns:
+
+### Standalone Callback Refs
+
+In the example above, the Callback Ref can be moved out of the function component. 
+Because the callback does not need to keep a ref anymore.
+
+```jsx
+function logClicks (node) {
+  if (!node) return;
+
+  const onClick = () => console.log('clicked!');
+
+  node.addEventListener('click', onClick);
+
+  return () => {
+    node.removeEventListener('click', onClick);
+  };
+}
+
+function MyComponent(props) {
+  return <button ref={logClicks} />;
+}
+```
+
+### Register pattern
+
+The same Callback Ref can be used for multiple elements.
+
+```jsx
+function MyComponent(props) {
+  return <>
+    <button ref={logClicks} />
+    <button ref={logClicks} />
+    <button ref={logClicks} />
+  </>;
+}
+```
+
+For example, we can write a Callback Ref that adds all elements to an array, 
+and remove them from the array when they are removed from DOM:
+
+```jsx
+function Test() {
+  const listRef = useRef([]);
+
+  const register = useCallback((el) => {
+    if (!el) return;
+
+    listRef.current.push(el);
+    
+    return () => {
+      const index = listRef.current.indexOf(el);
+      if (index >= 0) listRef.current.splice(index, 1);
+    };
+  }, []);
+
+  return <>
+    {[0,1,2,3,4,5].map((x,i) => (
+        <span key={i} ref={register} />
+    )}
+  </>;
+}
+```
+
+See the section `Alternatives` for how `react-hook-form` uses the `register` pattern.
+
+# Detailed design
+
+The general design of the feature looks like this:
+
+```jsx
+<div ref={node => {
+  // Normal ref callback
+
+  return () => {
+    // Cleanup function which is called when the ref is removed or changed
+    // This should not be called when `node` is null
+  };
+}} />
+```
+
+To keep backward compatibility, Callback Refs should still be called with `null` 
+when the element unmounts. But in this case, the value returned from Callback Ref 
+should be ignored, i.e. the cleanup function should never be called.
+
+No changes needs to be done to the other ref types, mutable refs and string refs.
+
+# Drawbacks
+
+- Existing codebases may be affected from this change. See the section `Adoption strategy` 
+for counter measures.
+- This feature will encourage a code pattern where a single Callback Ref can be 
+used for multiple nodes. It should be confirmed that this conforms to the React
+design principles.
+
+# Alternatives
+
+Users have created custom hooks by using `useRef` and `useCallback`. Basically, 
+a mutable ref holds the reference to the cleanup function that was returned 
+from the Callback Ref, and that cleanup function is called the next time when the 
+Callback Ref is called with a null. 
+See implementation of [@huse/effect-ref](https://github.com/ecomfe/react-hooks/blob/master/packages/effect-ref/src/index.ts)
+for such an example.
+
+However, none of the alternatives solve the problem introduced by `register` pattern. 
+Although not as a drop-in replacement, workarounds exist. One of the libraries that use
+this pattern is `react-hook-form`, and it solves this issue by making `register` a function
+that returns the actual Callback Ref when called with a unique ID:
+
+```
+<input ref={register("email")} />
+<input ref={register("password")} />
+```
+
+In the previous versions of `react-hook-form`, the API used to look like:
+
+```
+<input name="email" ref={register} />
+<input name="password" ref={register} />
+```
+
+This API was abandoned, presumably because how hard it is to maintain a registry of
+elements without a cleanup function. Note that we are not discussing which API 
+looks better. But having the cleanup functionality will be useful for other library 
+developers as well. Also, unlike this example, it is not always convenient to come 
+up with unique names for each element.
+
+# Adoption strategy
+
+This is a new feature and it is not a breaking change. However, existing projects may 
+still be affected if they have been returning a value from a Callback Ref. This would 
+be a no-op in previous React versions, but could result in errors in the new version.
+
+To be on the safe side, adoption of this feature can be done in two steps:
+
+- Modify the React source code so that returning a value from a Callback Ref is 
+forbidden. We can also add Flow and Typescript types to statically catch such situations.
+- Implement this feature, preferably in a major release.
+
+Alternatively, a codemod can check if users have been returning anything from Callback Refs and warn them before updating React.
+
+One advanced alternative which eliminates any breaking changes, and which may be an 
+overkill, is creating a new hook. This new hook will return an object which uses 
+an internal Symbol to keep a reference to cleanup function. If this symbol does not
+exist in the return value of Callback Ref, it will keep being a no-op.
+
+# How we teach this
+
+Developers are already familiar with the cleanup feature of `useEffect` hook. It should be 
+easy to teach people how to use this feature by explaining it in the same way as 
+[how cleanup up of useEffect works](https://reactjs.org/docs/hooks-reference.html#cleaning-up-an-effect).
+
+The phrase "Callback Ref cleanup" already gives an impression of what this feature does. 
+The part of documentation that needs to be updated is the section where 
+[Callback Refs](https://reactjs.org/docs/refs-and-the-dom.html#callback-refs) 
+are explained.
+
+# Unresolved questions
+
+- What should happen when Callback Refs return something other than a function?
+- Should there be a change to how Callback Refs are called with `null`?

From 14b2c226db9274a14022b17511531596846ece92 Mon Sep 17 00:00:00 2001
From: Gokhan Kurt <krtgkn@gmail.com>
Date: Sun, 12 Sep 2021 23:37:03 +0300
Subject: [PATCH 2/3] add detailed design

---
 text/0000-callback-ref-cleanup.md | 151 ++++++++++++++++++++++++------
 1 file changed, 124 insertions(+), 27 deletions(-)

diff --git a/text/0000-callback-ref-cleanup.md b/text/0000-callback-ref-cleanup.md
index 80f9c25b..6c35115c 100644
--- a/text/0000-callback-ref-cleanup.md
+++ b/text/0000-callback-ref-cleanup.md
@@ -1,6 +1,6 @@
 - Start Date: 2021-09-08
-- RFC PR: (leave this empty)
-- React Issue: (leave this empty)
+- RFC PR: https://github.com/reactjs/rfcs/pull/205
+- React Issue: https://github.com/facebook/react/issues/15176
 
 # Summary
 
@@ -31,8 +31,8 @@ function MyComponent(props) {
 Similar to `useEffect`, Callback Refs often create resources that need cleaning up
 when an element is unmounted. In such situations, developers often keep a reference to
 the element in a mutable ref, and clean it up the next time Callback Ref is 
-called with a `null` (See section `Alternatives`). As in the basic example above, 
-it is easy to clean up the effects of a Callback Ref  when the element is already in the 
+called with a `null` (See section `Alternatives`). But, as the example above shows, 
+it is easier to clean up the effects of a Callback Ref when the element is already in the 
 scope of the cleanup function.
 
 This feature also opens the way for two useful code patterns:
@@ -112,27 +112,62 @@ The general design of the feature looks like this:
 
   return () => {
     // Cleanup function which is called when the ref is removed or changed
-    // This should not be called when `node` is null
   };
 }} />
 ```
 
-To keep backward compatibility, Callback Refs should still be called with `null` 
-when the element unmounts. But in this case, the value returned from Callback Ref 
-should be ignored, i.e. the cleanup function should never be called.
+The behavior of the cleanup function should be similar to the behavior of the
+cleanup function of `useEffect`. Some points to keep in mind are:
+- The return value of the Callback Ref should be ignored if it's not a function.
+The return value should be called only if it is a function and there should not be
+an error when it is `null`, `undefined` or another type that is not a function.
+- It should be possible to conditionally return a cleanup function. This means the user
+can choose not to have a cleanup function for specific values of a ref.
 
-No changes needs to be done to the other ref types, mutable refs and string refs.
+No changes needs to be done to the other ref types, that is mutable refs and string refs.
+
+## Handling of the last null value
+
+In the current implementation, when a ref is unmounted, the Callback Ref is called 
+with `null`. To keep backward compatibility, Callback Refs should still be called 
+with `null` when the element unmounts.
+
+The cleanup function should be called when the value of a ref changes from one value to 
+another. This means that for the following code:
+
+```jsx
+function exampleCallbackRef(el) {
+  console.log('Callback ref called for:', el);
+  return () => console.log('Cleanup called for:', el);
+}
+
+...
+
+<button ref={exampleCallbackRef} />
+```
+
+The lifecycle of the Callback Ref should look like:
+
+```
+Callback ref called for: <button>
+Cleanup called for: <button>
+Callback ref called for: null
+```
+
+Note that the cleanup function is not called for `null` because it is the last value of this ref.
 
 # Drawbacks
 
-- Existing codebases may be affected from this change. See the section `Adoption strategy` 
-for counter measures.
+- Although very unlikely, existing codebases may be affected from this change. 
+See the section `Adoption strategy` for counter measures.
 - This feature will encourage a code pattern where a single Callback Ref can be 
 used for multiple nodes. It should be confirmed that this conforms to the React
 design principles.
 
 # Alternatives
 
+## Existing workarounds
+
 Users have created custom hooks by using `useRef` and `useCallback`. Basically, 
 a mutable ref holds the reference to the cleanup function that was returned 
 from the Callback Ref, and that cleanup function is called the next time when the 
@@ -140,7 +175,7 @@ Callback Ref is called with a null.
 See implementation of [@huse/effect-ref](https://github.com/ecomfe/react-hooks/blob/master/packages/effect-ref/src/index.ts)
 for such an example.
 
-However, none of the alternatives solve the problem introduced by `register` pattern. 
+However, none of the alternatives solve the problem introduced by the `register` pattern. 
 Although not as a drop-in replacement, workarounds exist. One of the libraries that use
 this pattern is `react-hook-form`, and it solves this issue by making `register` a function
 that returns the actual Callback Ref when called with a unique ID:
@@ -163,24 +198,84 @@ looks better. But having the cleanup functionality will be useful for other libr
 developers as well. Also, unlike this example, it is not always convenient to come 
 up with unique names for each element.
 
-# Adoption strategy
+## Alternative designs
 
-This is a new feature and it is not a breaking change. However, existing projects may 
-still be affected if they have been returning a value from a Callback Ref. This would 
-be a no-op in previous React versions, but could result in errors in the new version.
+### Wrapping the callback with a symbol
 
-To be on the safe side, adoption of this feature can be done in two steps:
+One advanced alternative which eliminates any breaking changes, and which may be an 
+overkill, is creating a new hook (`useCallbackRefWithCleanup`). This new hook will 
+return an object which uses an internal Symbol (`INTERNAL_SYMBOL_REF_CLEANUP`) to keep 
+a reference to cleanup function. If this symbol does not exist in the return value of 
+Callback Ref, it will be ignored. The cleanup function will behave the same as originally 
+described above.
+
+```js
+useCallbackRefWithCleanup((el) => {
+  /* effect code */
+  return () => { /* cleanup code */ };
+});
+
+// The above code will be the same as:
+
+useCallback((el) => {
+  /* effect code */
+  return { [INTERNAL_SYMBOL_REF_CLEANUP]: () => { /* cleanup code */ } };
+});
+```
 
-- Modify the React source code so that returning a value from a Callback Ref is 
-forbidden. We can also add Flow and Typescript types to statically catch such situations.
-- Implement this feature, preferably in a major release.
+Since `INTERNAL_SYMBOL_REF_CLEANUP` does not exist in the old React versions, there
+is no chance that existing codebase will break because of this change.
 
-Alternatively, a codemod can check if users have been returning anything from Callback Refs and warn them before updating React.
+### Introducing a new ref type
 
-One advanced alternative which eliminates any breaking changes, and which may be an 
-overkill, is creating a new hook. This new hook will return an object which uses 
-an internal Symbol to keep a reference to cleanup function. If this symbol does not
-exist in the return value of Callback Ref, it will keep being a no-op.
+Another alternative is to introduce a new ref type which eliminates confustion 
+caused by calling the ref with `null` when the element unmounts.
+
+The main problem with the current implementation of `ref` is that it is not possible to
+always know which element was unmounted. So another ref type may be needed. 
+This ref type may have the signature (Typescript):
+
+```tsx
+interface RegisterRef<T> {
+  register: (ref: T) => void;
+  unregister?: (ref: T) => void;
+}
+```
+
+And this can be used like:
+
+```jsx
+const onClick = () => console.log('clicked!');
+
+const logClicks = {
+  register: (node) => node.addEventListener('click', onClick);
+  unregister: (node) => node.removeEventListener('click', onClick);
+};
+
+function MyComponent(props) {
+  return <button ref={logClicks} />;
+}
+```
+
+Instead of `register/unregister`, other terminology like `mount/unmount` or `connect/disconnect`
+can also be used.
+
+Just like `MutableRef`, which uses `current` property to keep the reference of the element, 
+this ref type is also object typed. React should check if the `register` property of the ref is 
+a function to disambiguate between `RegisterRef` and `MutableRef`.
+
+Although this solution is quite different than what is discussed in this RFC, it solves the 
+same problems. It also eliminated any breaking changes. However, it can be a more significant 
+change and require more changes in documentation.
+
+# Adoption strategy
+
+This is a new feature and it is not a breaking change. However, existing projects may 
+still be affected if they have been returning a function from a Callback Ref. This would 
+be a no-op in previous React versions, but could result in errors in the new version.
+
+A codemod can check if users have been returning a function from Callback Refs and warn them
+to manually remove return valeus from Callback Refs before updating React.
 
 # How we teach this
 
@@ -195,5 +290,7 @@ are explained.
 
 # Unresolved questions
 
-- What should happen when Callback Refs return something other than a function?
-- Should there be a change to how Callback Refs are called with `null`?
+- What should happen when a ref is unmounting? To keep the backward compatibility, the Callback Ref should still be called with `null`.
+But, should the cleanup function be called, and if so, when should it be called?
+- There are multiple alternative solutions. Which one is more suitable?
+- The original proposal will not be a breaking change for the majority of codebases (I would say %99.9). Do we still need alternatives?

From cb1c4869b415945c985697d477fbfb2201ad38a9 Mon Sep 17 00:00:00 2001
From: Gokhan Kurt <krtgkn@gmail.com>
Date: Sun, 12 Sep 2021 23:52:17 +0300
Subject: [PATCH 3/3] remove some code for conciseness

---
 text/0000-callback-ref-cleanup.md | 82 ++++++-------------------------
 1 file changed, 14 insertions(+), 68 deletions(-)

diff --git a/text/0000-callback-ref-cleanup.md b/text/0000-callback-ref-cleanup.md
index 6c35115c..76b329ed 100644
--- a/text/0000-callback-ref-cleanup.md
+++ b/text/0000-callback-ref-cleanup.md
@@ -10,19 +10,12 @@ Callback Refs should allow returning a cleanup function which will be called whe
 
 ```jsx
 function MyComponent(props) {
-  const logClicks = useCallback((node) => {
-    if (!node) return;
-
-    const onClick = () => console.log('clicked!');
-
-    node.addEventListener('click', onClick);
-
-    return () => {
-      node.removeEventListener('click', onClick);
-    };
+  const exampleCallbackRef = useCallback((el) => {
+    /* effect code */
+    return () => { /* cleanup code */ };
   }, [])
 
-  return <button ref={logClicks} />;
+  return <button ref={exampleCallbackRef} />;
 }
 ```
 
@@ -35,72 +28,25 @@ called with a `null` (See section `Alternatives`). But, as the example above sho
 it is easier to clean up the effects of a Callback Ref when the element is already in the 
 scope of the cleanup function.
 
-This feature also opens the way for two useful code patterns:
-
-### Standalone Callback Refs
-
-In the example above, the Callback Ref can be moved out of the function component. 
-Because the callback does not need to keep a ref anymore.
+The lack of a cleanup mechanism becomes a problem when trying to use the same Callback Ref 
+for multiple elements. Consider the following example:
 
 ```jsx
-function logClicks (node) {
-  if (!node) return;
-
-  const onClick = () => console.log('clicked!');
-
-  node.addEventListener('click', onClick);
-
-  return () => {
-    node.removeEventListener('click', onClick);
-  };
-}
-
-function MyComponent(props) {
-  return <button ref={logClicks} />;
+function exampleCallbackRef (el) {
+  /* effect code */
+  return () => { /* cleanup code */ };
 }
-```
-
-### Register pattern
-
-The same Callback Ref can be used for multiple elements.
 
-```jsx
 function MyComponent(props) {
   return <>
-    <button ref={logClicks} />
-    <button ref={logClicks} />
-    <button ref={logClicks} />
-  </>;
-}
-```
-
-For example, we can write a Callback Ref that adds all elements to an array, 
-and remove them from the array when they are removed from DOM:
-
-```jsx
-function Test() {
-  const listRef = useRef([]);
-
-  const register = useCallback((el) => {
-    if (!el) return;
-
-    listRef.current.push(el);
-    
-    return () => {
-      const index = listRef.current.indexOf(el);
-      if (index >= 0) listRef.current.splice(index, 1);
-    };
-  }, []);
-
-  return <>
-    {[0,1,2,3,4,5].map((x,i) => (
-        <span key={i} ref={register} />
-    )}
+    <button ref={exampleCallbackRef} />
+    <button ref={exampleCallbackRef} />
+    <button ref={exampleCallbackRef} />
   </>;
 }
 ```
 
-See the section `Alternatives` for how `react-hook-form` uses the `register` pattern.
+With the new feature, it will be possible to know which element is being unmounted in the cleanup code.
 
 # Detailed design
 
@@ -175,7 +121,7 @@ Callback Ref is called with a null.
 See implementation of [@huse/effect-ref](https://github.com/ecomfe/react-hooks/blob/master/packages/effect-ref/src/index.ts)
 for such an example.
 
-However, none of the alternatives solve the problem introduced by the `register` pattern. 
+However, none of the alternatives solve the problem introduced by using the same Callback Ref for multiple elements.
 Although not as a drop-in replacement, workarounds exist. One of the libraries that use
 this pattern is `react-hook-form`, and it solves this issue by making `register` a function
 that returns the actual Callback Ref when called with a unique ID: