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

[Question] How would you handle conditional rendering with hooks? #162

Closed
justinhelmer opened this issue Feb 6, 2019 · 13 comments · Fixed by #168
Closed

[Question] How would you handle conditional rendering with hooks? #162

justinhelmer opened this issue Feb 6, 2019 · 13 comments · Fixed by #168
Labels

Comments

@justinhelmer
Copy link

justinhelmer commented Feb 6, 2019

First, I'd like to say thank you for such a great tool. Very useful.

I ran into a situation I wanted to run by you, and see if you have any suggestions.

This is specifically when using functional components with conditional rendering.

Consider the following scenario:

export default () => {
  const ref = useRef();
  const inView = useInView(ref);
  const [isLoading, setIsLoading] = useState(true);

  useEffect(() => {
    setIsLoading(false);
  }, []);

  if (isLoading) {
    return <div>Loading...</div>;
  }

  return <div ref={ref}>in view - {inView ? 'yes' : 'no'}</div>
};

In the above example, ref.current won't exist for the initial render. When the effect is run in useInView, it will fail to attach the observer.

When the isLoading state changes, a re-render will happen. However, the useInView effect is only run if one of the options changes.

Even if we add ref.current to the dependency array in the effect, this doesn't help. The reason being, the dependencies are checked against at the time of render, whereas react refs get modified after rendering. See facebook/react#14387 (comment).

If we remove the list of dependencies entirely, we can see the ref.current has the proper reference in useEffect on the subsequent render. This is because the hook execution is deferred until after render (and thus the reference has been updated). However this results in an infinite render loop for this scenario. This may be the reason you added dependencies to useEffect to begin with.


I apologize for not coming forward with a potential solution, but this one stumped me. useLayoutEffect doesn't help us here either, because the inView state is set asynchronously.

One "workaround" is to always include the ref in the rendered component, i.e.

if (isLoading) {
  return <div>Loading...<div ref={ref}></div>;
}

return <div ref={ref}>in view - {inView ? 'yes' : 'no'}</div>

However, this is not ideal and comes with its own set of pitfalls/challenges.

It may also be worth noting that this is a non-issue when using render props, since the ref is guaranteed to exist when the observer is attached.

@thebuilder
Copy link
Owner

thebuilder commented Feb 6, 2019

That's interesting! Thanks for the detailed description.
I guess issues like this is to be expected with hooks to start with.

@thebuilder
Copy link
Owner

The question is, how to handle when the ref changes, and should we handle it?

  • It could be undefined to begin with, and then set later.
  • It could change to a different element.
  • It could change from an element to undefined

It needs to handle observering and unobservering whenever it changes.

@thebuilder
Copy link
Owner

I'm currently leaning towards throwing an error if the ref is not the set when you create the hook.

Looking at your example, i think it would make sense for you to create a <Loading> component and a <Result> component. You can then move the the useInView hook inside the Result.

The reason is you aren't actually using the hook before you load the content.

@justinhelmer
Copy link
Author

makes sens @thebuilder. May result in a pretty superficial component, but it does seem like the right solution (and certainly the easiest). Feel free to close this issue.

@thebuilder
Copy link
Owner

I looked a bit more into how to solve this, and made a solution that recreates the observer when the ref changes. I do this by updating a useState with the current ref in a new useEffect call that always runs. This state can then be used in second useEffect to trigger a new render.

https://github.com/thebuilder/react-intersection-observer/blob/master/src/hooks.tsx#L25

  React.useEffect(() => {
    if (ref.current !== currentRef) {
      setCurrentRef(ref.current)
    }
  })

It seems like a more robust solution, that's less likely to cause surprises. Also had a coworker with a similar issue last week, so it seems like a common enough use case.

@justinhelmer
Copy link
Author

elegant

@raRaRa
Copy link

raRaRa commented Feb 11, 2019

I looked a bit more into how to solve this, and made a solution that recreates the observer when the ref changes. I do this by updating a useState with the current ref in a new useEffect call that always runs. This state can then be used in second useEffect to trigger a new render.

https://github.com/thebuilder/react-intersection-observer/blob/master/src/hooks.tsx#L25

  React.useEffect(() => {
    if (ref.current !== currentRef) {
      setCurrentRef(ref.current)
    }
  })

It seems like a more robust solution, that's less likely to cause surprises. Also had a coworker with a similar issue last week, so it seems like a common enough use case.

The useEffect will run on every re-render, wouldn't it be better if it only runs when ref.current changes? E.g.

  React.useEffect(() => {
    if (ref.current !== currentRef) {
      setCurrentRef(ref.current)
    }
  }, [ref.current])

@thebuilder
Copy link
Owner

@raRaRa That's the problem, the hook won't trigger when ref.current changes.

I made a blog post about how I solved this issue - Using a ref callback instead of useRef.
https://medium.com/@teh_builder/ref-objects-inside-useeffect-hooks-eb7c15198780

@raRaRa
Copy link

raRaRa commented Feb 11, 2019

Ah wow, good catch! Thanks :)

@thebuilder
Copy link
Owner

You also shouldn't really worry about running an inexpensive useEffect on every render - It is the recommended way, in order to avoid bugs like this.

@justinhelmer
Copy link
Author

I had an issue with moving the ref to a child component, but it looks like you already thought of that and handled it with callback refs in the new 8.x version! 👏

@theairbend3r
Copy link

theairbend3r commented Jun 26, 2020

Sorry to post on a closed issue. I think I have an error directly related to this. I am unable to solve it. The question is on stackoverflow - React state is always one step behind while making predictions on uploaded image using Tensorflowjs.

I'm using useRef to pass the image to tfjs using useEffect. But instead of the prediction happening for the current image, I get the prediction for the previous image. Would greatly appreciate if you can have look.

@gsevla
Copy link

gsevla commented Mar 25, 2021

@raRaRa That's the problem, the hook won't trigger when ref.current changes.

I made a blog post about how I solved this issue - Using a ref callback instead of useRef.
https://medium.com/@teh_builder/ref-objects-inside-useeffect-hooks-eb7c15198780

Hey man, thanks for the article!

I have an use case that I need to pass back some things back to the main component from the component that have ref.

Usually I could achieve that using the useImperativeHandle hook but I couldn't make it work with this solution.

I have something like:

const C1 = () => {
    const [ref, refReady] = useCallbackRef();

    useEffect(() => {
        if(refReady) {
            // break here because now ref is a function and not a object, so what should i do to call setActiveItem?
            
        }
    }, [refReady])

return (
    <C2Fw ref={ref} />
>
}

const C2 = ({innerRef}) => {
    const activeItem = useRef(); //tried using useState too

    const setActiveItem = (itemId) => {
        activeItem.current = itemId
    }

    useImperativeHandle(innerRef, () => ({
        setActiveItem,
        activeItem: activeItem.current
    }))
}

const C2Fw = forwardRef((props, ref) => (<C2 innerRef={ref} {...props} />));

My problem is that I need some values from inside C2 to use on C1 and make it render on changes.

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

Successfully merging a pull request may close this issue.

5 participants