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

Fix stale localState #44

Merged
merged 2 commits into from
Feb 19, 2020
Merged

Conversation

mayteio
Copy link
Contributor

@mayteio mayteio commented Feb 17, 2020

I noticed that when keys dynamically change and re-render, localState is stale. The following example demonstrates this currently:

const App = () => {
  // key starts out as one value
  const [key, set] = React.useState('key1');

  // pass in key to get localStorage
  let [name, setName] = useLocalStorage(key, 'default value');
  React.useEffect(() => {
    // immediately set localStorage value to something
    setName('key1 name');

    // update the key and re-render
    setTimeout(() => set('key2'), 1000);

    // resulted in the {name} below always printing "key1 name"
  }, []);

  return (
    <div>
      <h1>{name}</h1>
    </div>
  );
};

This pull request fixes this by basically adding the key as a dependency to each useEffect/useCallback, and adding an effect that rehydrates localState when the key changes with whatever the actual value is.

I also wrote a test in test/index.test.tsx to ensure it works. Current tests don't catch a stale onLocalStorageChange useCallback at the moment either - worth noting.

Finally - I apologies for the handful of formatting changes - there is no .prettierrc file to enforce code style.

Let me know if you have any questions! Cheers for the great plugin otherwise.

@mayteio mayteio changed the title Fix stale local state Fix stale localState Feb 17, 2020
@iamsolankiamit iamsolankiamit merged commit 7cf2f3b into rehooks:master Feb 19, 2020
@mayteio mayteio deleted the fix-stale-localState branch February 20, 2020 07:47
@mayteio mayteio restored the fix-stale-localState branch February 23, 2020 21:48
Copy link
Contributor Author

@mayteio mayteio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamsolankiamit this didn't seem to make it into master.


useEffect(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @iamsolankiamit I don't think this particular useEffect made it into the release for some reason? It's missing from compiled source and master.

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

Successfully merging this pull request may close these issues.

3 participants