Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useEffect(effect, [ref.current]) is prematurely re-running #14387

Closed
1000hz opened this issue Dec 4, 2018 · 17 comments
Closed

useEffect(effect, [ref.current]) is prematurely re-running #14387

1000hz opened this issue Dec 4, 2018 · 17 comments

Comments

@1000hz
Copy link

1000hz commented Dec 4, 2018

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
It seems useEffect(effect, [ref.current]) is re-running under certain circumstances without the current ref identity changing.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React:
https://codesandbox.io/s/qkzl9xjj44

What is the expected behavior?
In this example, the effect should only be run once, not re-run (and thus cleaned up) upon clicking the button for the first time. The isActive state should be returned to false on mouseup. CLEAN UP! should not be logged to the console since the ref hasn't been reassigned.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
16.7.0-alpha.2 (affects all environments)

@brookslybrand
Copy link

brookslybrand commented Dec 5, 2018

I don't believe this is actually a bug. The problem is, ref is not actually defined when you pass it in, but since you're passing a reference that gets assigned, it actually gets assigned when the effect kicks off.

Where you have [ref.current] in your useEffect call (line 28), try replacing it with this and and you'll see what I mean:

[
  (() => {
    console.log("ref to compare with", ref.current);
    return ref.current;
  })()
]

The ref is undefined when you first pass it because the component hasn't actually mounted, and thus it has nothing to reference.

This simple solution is to this is to use useLayoutEffect, since that will ensure that the ref is in fact referencing the DOM node.

The signature [useLayoutEffect] is identical to useEffect, but it fires synchronously after all DOM mutations.

@1000hz
Copy link
Author

1000hz commented Dec 5, 2018

Ah, right. React checks the effect dependencies during render, but defers running the effect until after commit/paint. So it doesn't recheck the deps until the next render, which happens after setIsActive(true).

So while I guess this isn't a bug per se, it still might be something that could be addressed. Maybe after commit, React could re-run the render function to update the dependencies (but not actually re-render) if it's not too expensive and doesn't cause any other unwanted side effects. I did notice useLayoutEffect happened to skirt around the issue in this particular example, but I'm more concerned with fixing this unintuitive behavior if possible than fixing the implementation of that hook.

@1000hz
Copy link
Author

1000hz commented Dec 5, 2018

Since this is only a problem with mutable values used as deps, and mutable values should be captured inside of a ref, perhaps React can special case the dependency diffing for refs to check the current value. So you would call useEffect(effect, [ref]) and React would know to compare the previous value to whatever ref.current is.

@gaearon
Copy link
Collaborator

gaearon commented Jan 18, 2019

We'll need to add this to FAQ, it's an unfortunate confusing aspect. [ref.current] (or any mutable field) is going to be problematic when included in dependencies because there's no guarantee it would actually be changed before rendering (or that changing it would cause a rendering). In case of React refs, they get modified after rendering which is why it always lags behind.

We're working on a lint rule for this but let's keep this open so we don't forget to update the docs too.

@thebuilder
Copy link

So I also ran into this issue on my first attempt at a custom hook.

One solution is to use the old ref callback method instead. This allows you to store the ref node in a useState, so you can add it to the useEffect update dependencies.

import React, {useState} from 'react'

function useHookWithRefCallback() {
  const [node, setRef] = useState(null)

  useEffect(
    () => {
      if (node) {
        // Your Hook now has a reference to the ref element.
      }
    },
    [node],
  )

  return [setRef]
}

function Component() {
  // In your component you'll still recieve a `ref`, but it 
  // will be a callback function instead of a Ref Object
  const [ref] = useHookWithRefCallback()
  
  return <div ref={ref}>Ref element</div>
}

I ended up making a Medium post about the problem: https://medium.com/@teh_builder/ref-objects-inside-useeffect-hooks-eb7c15198780

@mike-marcacci
Copy link

mike-marcacci commented May 18, 2019

Just to be clear here, useRef does invalidate the component and cause a re-render when the ref is bound by react, correct?

(If not, I suppose it could be used in conjunction with useState and a Proxy, similarly to @thebuilder's callback-based workaround above.)

EDIT: Nope, it appears that it does not, and the actual createRef definition is far less magical than I was expecting 🤨... This seems quite easy to solve on my side, but it really makes me question what the use of useRef really is.

@mike-marcacci
Copy link

mike-marcacci commented May 18, 2019

I'm now really struggling to understand why useRef is a thing when (as far as I can tell) this would have the exact same effect:

const [ ref ] = useState({current: null})

I had assumed useRef worked more like this:

function useCustomRef<T>(d: T): { current: T } {
  const [value, setValue] = useState<T>(d);
  const [ref] = useState({
    set current(value) {
      setValue(value);
    },

    get current(): T {
      return value;
    }
  });

  return ref;
}

@gaearon
Copy link
Collaborator

gaearon commented May 18, 2019

No, useRef works more like this:

function useRef(initialValue) {
  const [ref, ignored] = useState({ current: initialValue })
  return ref
}

So setting the current field would never trigger a re-render.

@gaearon
Copy link
Collaborator

gaearon commented May 18, 2019

When you try to put ref.current in dependencies, you usually want a callback ref instead: https://reactjs.org/docs/hooks-faq.html#how-can-i-measure-a-dom-node

@switz
Copy link

switz commented May 18, 2019

edit: I totally responded to the wrong thread. My bad, apologies for wasting time.

@anion155
Copy link

Worked for me:

function useCorrectRef(initial) {
  const [value, setValue] = useState(initial);
  const [ref, setRef] = useState({ current: initial });
  useEffect(() => {
    setRef({
      get current() {
        return value;
      },
      set current(next) {
        setValue(next);
      }
    });
  }, [value]);
  return ref;
}

@gaearon
Copy link
Collaborator

gaearon commented Jun 19, 2019

@anion155 This is not a correct solution. If you want to use state, just use state directly. Refs are for mutable values that are intended to be mutated outside the render data flow. If you want them to cause a re-render, you shouldn't be using a ref.

I'm going to close this thread. As I mentioned earlier, if you put [ref.current] in dependencies, you're likely making a mistake. Refs are for values whose changes don't need to trigger a re-render.

If you want to re-run effect when a ref changes, you probably want a callback ref instead. This is described here, for example.

@gaearon gaearon closed this as completed Jun 19, 2019
@anion155
Copy link

anion155 commented Jun 19, 2019

@gaearon Okay. Than if I would need to pass canvas to some couple of hand made hooks, to create some objects in imperative style, will this code be correct? (I know it's not SO, but it was a question I made this kind of solution):

function useViewFabric(transport: Socket | null, canvas: HTMLCanvasElement | null) {
  const [view, set] = useState(null);
  useEffect(() => {
    if (transport && canvas) {
      const created = new View(transport, canvas);
      set(created);
      return () => created.dispose();
    }
  }, [transport, canvas]);
  return view;
}

function CanvasView() {
  const [canvas, setCanvas] = useState(null);
  const transport = useContext(TransportContext);
  const view = useViewFabric(transport, canvas);
  return <canvas ref={setCanvas} />;
}

function App() {
  return <><CanvasView /><CanvasView /><>;
}

Or is there a better approach?

@1000hz
Copy link
Author

1000hz commented Jun 19, 2019

As I mentioned earlier, if you put [ref.current] in dependencies, you're likely making a mistake. Refs are for values whose changes don't need to trigger a re-render.

This is great insight. Using the callback ref pattern seems to solve this issue. I made a const [node, ref] = useNodeRef() hook to give me a proper reference to the node. I've updated the example here:
https://codesandbox.io/s/sxkq6

Thanks @gaearon!

EDIT: useNodeRef is functionally equivalent to useState ...!
You could just as well do const [node, ref] = useState(). Maybe this is bad?

@likern
Copy link

likern commented Feb 3, 2020

@anion155 This is not a correct solution. If you want to use state, just use state directly. Refs are for mutable values that are intended to be mutated outside the render data flow. If you want them to cause a re-render, you shouldn't be using a ref.

I'm going to close this thread. As I mentioned earlier, if you put [ref.current] in dependencies, you're likely making a mistake. Refs are for values whose changes don't need to trigger a re-render.

If you want to re-run effect when a ref changes, you probably want a callback ref instead. This is described here, for example.

@gaearon I think this is not always true. useRef represents two different concepts: instance-like variable for functional component and reference to mounted component itself.

For example, I have this custom hook:

// file1.ts
export function useImmerRef(initialValue: any, deps?: DependencyList) {
  const valueRef = useRef<any>(null);
  const updateFunction: any = useMemo(
    () => {
      valueRef.current = produce(initialValue, (_draft: any) => {});
      return (updater: any) => {
        const newState = produce(initialValue, updater);
        valueRef.current = newState;
        return newState;
      };
    },
    
    // eslint-disable-next-line react-hooks/exhaustive-deps
    deps
  );
  return [valueRef, updateFunction];
}

// file2.tsx
const [tagsRef, updateTags] = useImmerRef(tags, []);

Here I'm using useRef as instance variable and it's different from reference to mounted component.

  1. It's never get assigned null value
  2. We need to use useRef to be able to substitute .current to new reference, so changes would be observable (this is the problem with implementation using useState)

If I use original useImmer implementation which internally uses useState - reference can be updated only on next rendering on this line

// tags is Map instance
const [tagsState, updateTagsState] = useImmer(tags)

otherwise it's not possible to update tagsState reference. That leads to bug that changes are not observable:

updateTagsState(draft => {
  const tag = draft.get(data.id);
  if (tag !== undefined) tag.isSelected = data.isSelected;
});
// Here tagsState has reference to the old structure, 
// it's not possible to observe changes until next render
const tag = tagsState.get(data.id);

I think it's totally valid to use ref.current as dependency in this case. But it turns out it's not working as I expected.

ptgolden added a commit to dredge-bio/dredge that referenced this issue Apr 17, 2021
I'm not 100% sure I'm doing this right, but here is what I did, and here
is why I did it.

1) I started passing refs to DOM elements to the hooks rather than
`hook.current`, and I took took that ref out of the dependency list
for the hooks. The latter is easy: `ref.current` *never* changes because
it always refers to the same object, so it doesn't make sense to have it
as an effect dependency.

<facebook/react#14387 (comment)>

`useEffect` runs *after* the <svg> node has already been added to the
DOM, so now we don't have to check if it is `null`.

2) I'm using `useLayoutEffect` rather than `useEffect`, because all of
these hooks affect the state of the DOM-- they are just being managed by
D3 rather than React.
@shiyuangu
Copy link

shiyuangu commented Sep 22, 2022

I don't believe this is actually a bug. The problem is, ref is not actually defined when you pass it in, but since you're passing a reference that gets assigned, it actually gets assigned when the effect kicks off.

Where you have [ref.current] in your useEffect call (line 28), try replacing it with this and and you'll see what I mean:

[
  (() => {
    console.log("ref to compare with", ref.current);
    return ref.current;
  })()
]

The ref is undefined when you first pass it because the component hasn't actually mounted, and thus it has nothing to reference.

This simple solution is to this is to use useLayoutEffect, since that will ensure that the ref is in fact referencing the DOM node.

The signature [useLayoutEffect] is identical to useEffect, but it fires synchronously after all DOM mutations.

useLayoutEffect doesn't solve the problem in React18 (Cf: sandbox react-18 ) but it did in React16 (Cf: (sandbox-react16) ). The reason seems to be setIsActive(true) in the mousedown handler is not flushed in react18 as it was in react16.

@nickopicz
Copy link

I'm now really struggling to understand why useRef is a thing when (as far as I can tell) this would have the exact same effect:

const [ ref ] = useState({current: null})

I had assumed useRef worked more like this:

function useCustomRef<T>(d: T): { current: T } {
  const [value, setValue] = useState<T>(d);
  const [ref] = useState({
    set current(value) {
      setValue(value);
    },

    get current(): T {
      return value;
    }
  });

  return ref;
}

The use would be for a developer to pass values within sibling components, or to a parent without re-rendering the whole application. It has its use cases especially when rendering large lists and calling functions from within those lists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants