astoilkov / use-local-storage-state

React hook that persists data in localStorage
MIT License
1.17k stars 41 forks source link

Adding a `isLoading` property to `LocalStorageProperties` #40

Closed jpb06 closed 2 years ago

jpb06 commented 2 years ago

Hello! First of, thank you for this library 🙇🏻

I ran into an issue while using this lib. I'd need to store a value in local storage and do something with it once it is available. Let's see a quick example:

const MyComponent = () => {
  const [data] = useLocalStorageState<string>(
    'my-data', { ssr: true }
  );

  console.info('data is...', data);

  useEffect(() => {
    // I want to do something with data here 🧐
  }, [data]);

  // [...]
}

When we run this, we get the following result with my-data set to "cool":

data is... undefined
data is... cool

Nothing crazy here, we get the default value, which is not set. Now, what if I needed to execute that side effect only when the value is actually the one set in local storage?

I propose the following solution to solve that issue: adding a isLoading prop to LocalStorageProperties. Intended usage could look like this:

const MyComponent = () => {
  const [data,,{ isLoading }] = useLocalStorageState<string>(
    'my-data', { ssr: true }
  );

  useEffect(() => {
    if(!isLoading) {
      // I will have the actual value now! 🎉
    }
  }, [data, isLoading]);

  // [...]
}

What do you think about the idea? Let me know if I missed anything!

astoilkov commented 2 years ago

You can solve your issue without an extra property in the LocalStorageProperties. Here are two ways to do it:

What do you think?

jpb06 commented 2 years ago

Sorry, I forgot to mention this is typically an issue in a SSR scenario.

Sure thing, we could go even farther and check if the code is executed client side by using a global.window !== undefined condition.

But since this library is about providing a hook to interact with local storage, doesn't it sound logical to consider it is also this hook's responsibility to inform consumer real value is actually available?

astoilkov commented 2 years ago

Yes, it makes sense.

Can I ask you one more question? Why do you need the isLoading variable? Are you doing some kind of optimization where you don't want to, for example, send two requests to a server?

jpb06 commented 2 years ago

I'm working on a POC using an authentication mockup implementation: a jwt token is stored in local storage (Yeah... I know 🤡), following a successful signup. In pages requiring authentication, if token does not exist or user profile fetching fails, the user is redirected to signup.

So the scenario here is we might have a value in local storage that is either undefined or have a value. If it is undefined, we redirect right away... But the hook initially returns undefined (because we're in SSR) even though we might actually have something defined in local storage.

Perhaps isLoading is poor naming here for this piece of information. We're not loading anything, after all. isFetchedFromLocalStorage (verbose) or hasLoaded would better convey intent, maybe?

astoilkov commented 2 years ago

I'm experimenting with another approach that solves your issue. Give me some more time in order to figure out if it's a good idea.

Thanks for the understanding.

astoilkov commented 2 years ago

This is what I'm proposing as an implementation: https://github.com/astoilkov/use-local-storage-state/commit/762fe4f413d5bd0805eab7e8e22bf1b0875d7218 — it doesn't call the useEffect() and useLayoutEffect() callbacks on the first render.

The benefits of this approach are:

What do you think?

jpb06 commented 2 years ago

Looks great, thank you!