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

Adding a isLoading property to LocalStorageProperties #40

Closed
wants to merge 1 commit into from

Conversation

jpb06
Copy link

@jpb06 jpb06 commented Mar 6, 2022

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
Copy link
Owner

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

  • Don't set ssr to trueuseLocalStorageState<string>('my-data')
  • Use a useRef() to check if it's the first render:
const MyComponent = () => {
  const [data,,{ isLoading }] = useLocalStorageState<string>(
    'my-data', { ssr: true }
  );
  const isFirstRender = useRef(true)

  useEffect(() => {
    if(!isFirstRender.current) {
      // I will have the actual value now! 🎉
    }

    isFirstRender.current = false
  }, [data]);

  // [...]
}

What do you think?

@jpb06
Copy link
Author

jpb06 commented Mar 6, 2022

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
Copy link
Owner

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
Copy link
Author

jpb06 commented Mar 6, 2022

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
Copy link
Owner

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 added a commit that referenced this pull request Mar 11, 2022
@astoilkov
Copy link
Owner

This is what I'm proposing as an implementation: 762fe4f — it doesn't call the useEffect() and useLayoutEffect() callbacks on the first render.

The benefits of this approach are:

  • Better performance (as the code in the React effects won't be called twice)
  • Your scenario will automatically work without an additional condition
  • There won't be a need to add the isLoading property and thus the library API will be smaller/simpler

What do you think?

@astoilkov astoilkov added the enhancement New feature or request label Mar 12, 2022
@jpb06
Copy link
Author

jpb06 commented Mar 14, 2022

Looks great, thank you!

@jpb06 jpb06 closed this Mar 14, 2022
astoilkov added a commit that referenced this pull request Mar 14, 2022
@jpb06 jpb06 deleted the add-is-loading-prop-for-ssr branch March 17, 2022 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants