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

[BUG] useLocalStorage's setValue should be callable in render phase (like a normal useState) #549

Open
jraoult opened this issue Mar 19, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@jraoult
Copy link

jraoult commented Mar 19, 2024

Describe the bug

Calling the setter of useLocalStorage in a render phase triggers the error with the message Cannot call an event handler while rendering.. It is because useEventCallback is used internally, but I think useCallback should be used instead since it is not conceptually only an event handler.

To Reproduce

See the reproduction sandbox, but the gist of it is:

import { useEffect, useState } from 'react';
import { useLocalStorage } from 'usehooks-ts';

function App() {
  const [count, setCount] = useState(0);
  const [lsCount, setLsCount] = useLocalStorage('lsCount', 0);

  if (count === 0) {
    setCount(1);
  }

  // if (lsCount === 0) {
  //   // this should work but fails right now
  //   setLsCount(1);
  // }

  // this is the workaround
  useEffect(() => {
    if (lsCount === 0) {
      setLsCount(1);
    }
  }, [lsCount]);

Expected behavior

It should be possible to set a value in the render phase.

@jraoult jraoult added the bug Something isn't working label Mar 19, 2024
@LonelyFellas
Copy link

@jraoult #559

@juliencrn
Copy link
Owner

While I agree with the fact that it should be possible to set a value in the render phase, it's not a recommendable approach as the setter triggers a re-render every time the component is rendered. This can lead to performance issues and unexpected behavior, especially if you're updating state frequently or performing expensive operations.

This instead would be the right approach IMHO:

  useEffect(() => {
    if (lsCount === 0) {
      setLsCount(1);
    }
  }, [lsCount]);

@jraoult
Copy link
Author

jraoult commented Mar 28, 2024

@juliencrn one last attempt to convince you:

the setter triggers a re-render every time the component is rendered.

This can also happen with useEffect. The way you schedule the update doesn't prevent the recursion. What matters is the base case ie. the if (lsCount === 0).

it's not a recommendable approach

I think it is actually the recommended approach in the official React documentation (cf. https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes). They recommend not using an effect and just setting the value in the render phase: "Start by deleting the Effect. Instead, adjust the state directly during rendering"

@jraoult
Copy link
Author

jraoult commented Mar 28, 2024

@juliencrn I also just wanted to add that if you change your mind, I'm happy to create a PR with the proper test coverage for this.

@juliencrn juliencrn reopened this Apr 4, 2024
@juliencrn
Copy link
Owner

I think it is actually the recommended approach in the official React documentation (cf. https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes). They recommend not using an effect and just setting the value in the render phase: "Start by deleting the Effect. Instead, adjust the state directly during rendering"

Oh, I didn't know about that, interesting.

So, I'm open to considering a fix, thanks by advance!

@jraoult
Copy link
Author

jraoult commented Apr 4, 2024

@juliencrn great. Leave it with me. I'll take a look at this over the week end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants