Fix use local storage #786

TylerR909 · 2019-11-20T08:52:18Z

Addresses #785

Description

Basically a total rewrite of useLocalStorage to ensure updates to the keyed value are picked up by all components watching for it.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as before)

I don't believe this will cause any breaking changes. But I did end up tearing out almost all the old code and fundamentally changed how this hook works. 🤷‍♂

Checklist

TylerR909 · 2019-11-20T09:04:14Z

Hihi,

Although I only filed the one bug (#785) I was also running into other issues while trying to use this, like it over-serializing values ("foo" ==> '"\\"foo\\""') and some funkiness around initial state.

Running my test suite over the original hook gave these results:

Not sure why so many of the earlier ones are ❌, perhaps as a consequence of useEffect and many of those tests not rerendering (just checking the initial value). Those two ❌ in the middle though are the catalyst for this PR - Over-stringifying and hooks falling out of sync.

I'm more than happy to take a couple days to work with you to tighten the bolts on this one and get it ready to go! Let me know what else you'd like to see from me.

I also previously contributed #557 so I'm a little oversensitive to the react-hooks/rules-of-hooks eslint rules, thus the tests.

TylerR909 · 2019-11-20T09:09:52Z

src/useLocalStorage.ts

+    [state, raw]
+  );
+
+  useEffect((): void => {


I could probably swap this out for useEffectOnce

ayush987goyal · 2020-01-16T17:37:13Z

Hi @TylerR909 ,

Could you please rebase your branch from master. It seems like there have been more changes to the useLocalStorage hook. Maybe you can make changes on top of it?

Thanks

mayteio · 2020-01-30T03:59:14Z

Just ran into #785 and unfortunately have to find another implementation until this is merged. Be great to get it in there - let me know if I can help.

TylerR909 · 2020-01-31T03:55:36Z

I'll try to check this out in the next couple days and get the PR updated. Need to remind myself what I was doing when I opened it 2 months ago 🤷‍♂

roninjin10 · 2020-02-01T06:05:07Z

src/useLocalStorage.ts

    return [initialValue as T, () => {}];
  }
+  if (!key && (key as any) !== 0) {


Can it be a boolean false?

Sure, I can protect against that.

Not sure what I was asking here looking back.

roninjin10 · 2020-02-01T06:15:27Z

src/useLocalStorage.ts

-  });
+  let localStorageValue: string | null = null;
+  try {
+    localStorageValue = localStorage.getItem(key);


Isn't this expensive to be reading local storage every render?

Not really. If you're trying to hit 60fps that gives you 16ms between rerenders for all your code to run.

From various benchmarks I'm seeing, a single read takes 0.0004-0.0010ms. Completely negligible.

If reading localStorage is someone's bottleneck, they have other issues they need to figure out.

Ah interesting. I've seen multiple tutorials making a hook like this that cite reading local storage as being synchronous and expensive as the reason for wanting to make a hook. I assumed that was based on reality but it must have just been used as a contrived example.

I started on a PR that used an object store to keep all the hooks in sync while caching the data but if it's not actually an issue that is over-optimization

roninjin10 · 2020-02-01T06:28:00Z

IMO because reading from local storage is expensive, we should not be reading it on every rerender. I think since we want to keep it in sync we should cache the keys in an redux-like object shared by all instances of the hook rather than local state.

TylerR909 · 2020-02-02T00:51:33Z

Most of the way through updating this locally. I had added a bunch of tests cases back in November and some of the new Serializer stuff is missing the marks (feeding me back [object Object] instead of just... the string) and I've already spent a couple hours on a Saturday trying to resolve merge conflicts.

I'll circle back on this tomorrow and try to finish updating it.

roninjin10 · 2020-02-02T01:21:50Z

@TylerR909 One test case you can add that I think your diff fixes is that setting local storage works if it is called after unmount.


it('should properly update the localStorageOnChange when component unmounts', () => {
  const key = 'some_key';
  const updatedValue = { b: 'a' };
  const expectedValue = '{"b":"a"}';

  const { result, unmount } = renderHook(() => useLocalStorage(key));

  unmount();

  act(() => {
    result.current[1](updatedValue);
  });

  expect(localStorage.__STORE__[key]).toBe(expectedValue);
});

TylerR909 · 2020-02-03T05:28:13Z

Okie doke. Think this is mostly updated for now.

streamich

Thanks, I've rebased this onto v14.0 branch, I've kept the tests and updated the useLocalStorage in v14.0 branch to pass those.

The exception is one test, which arguably is the crux of this PR, to test that two hooks stay in sync as one updates. The reason is, because I could not understand the sync logic. As I understood it, the useLocalStorage hooks would somehow re-render and every time pick the latest value from localStorage—that would keep them in sync. I'm not sure that would work in general case, I would expect that useLocalStorage use some pub/sub or event emitter mechanism under the hood to notify all instances that some key has changed.

TylerR909 · 2020-02-04T00:15:14Z

The exception is one test, which arguably is the crux of this PR, to test that two hooks stay in sync as one updates. The reason is, because I could not understand the sync logic. As I understood it, the useLocalStorage hooks would somehow re-render and every time pick the latest value from localStorage—that would keep them in sync. I'm not sure that would work in general case, I would expect that useLocalStorage use some pub/sub or event emitter mechanism under the hood to notify all instances that some key has changed.

My concern is that someone is going to have the bright idea to come in here and "Optimize" useLocalStorage, all the tests are going to pass, and we're going to see a regression for something we specifically fixed in this PR. That is the test that is supposed to prevent this.

I disagree with commenting that test out. Even if someone changes the implementation to pub/sub, that test as written should still pass.

I'm not sure that would work in general case

Based on what? I did some research when someone else brought up the same concern and was seeing localStorage reads take 0.0004-0.0010ms which is basically negligible if you're targeting 60fps. If you have 20,000 components reading localStorage and it's going slow, your problem is not the speed of localStorage.

I could not understand the sync logic

Yeah, not my best code in the world. Basically it used to use const [state, setState] = useState(...);, but now it manually calculates const state = useMemo(...) and const setState = useCallback(...) with a useEffectOnce to call setState if the value in localStorage has never been set before.

Once you break it down into calc state and calc setState it's a little easier to reason around.

I don't recall what I was trying to use this hook on back in November when I made these updates but it rendered the hook entirely useless to me because one component was setting localStorage when a query string changed (or something) and some View component absolutely refused to see the change until remounting.

So yes this is an important test to include. Even if the implementation changes.

Fixes streamich#785 - useLocalStorage not updating if many components watch the same key Previously pr streamich#786 addressed this issue by ensuring that other components watching the key would see new updates, however, those updates would not be rendered until something else triggered a re-render. This pr resolves that issue. This pull request depends on pull requests streamich#1021 streamich#979

streamich · 2020-05-16T13:33:13Z

🎉 This PR is included in version 15.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Fixes streamich#785 - useLocalStorage not updating if many components watch the same key Previously pr streamich#786 addressed this issue by ensuring that other components watching the key would see new updates, however, those updates would not be rendered until something else triggered a re-render. This pr resolves that issue. This pull request depends on pull requests streamich#1021 streamich#979

Tyler Swavely added 7 commits November 19, 2019 19:59

add useLocalStorage tests

5cfddaf

finish tests and hook

da4bfdd

reject nullish keys

126c509

reintroduce types

ead2242

run prettier

affcbbd

add some ignores to tests

683e658

enforces rules of hooks

b702692

TylerR909 commented Nov 20, 2019

View reviewed changes

Tyler Swavely added 2 commits November 25, 2019 21:34

Merge remote-tracking branch 'upstream/master' into fix-useLocalStorage

948e541

initialState switched to useEffectOnce

c839644

streamich requested review from ankithkonda and streamich and removed request for ankithkonda December 9, 2019 23:47

roninjin10 reviewed Feb 1, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix-useLocalStorage

812d31a

Tyler Swavely added 3 commits February 2, 2020 21:21

add serializer options back to useLocalStorage

becdfff

linting

8d18ef9

remove unused file

e5f4167

streamich changed the base branch from master to v14.0 February 3, 2020 22:11

streamich added 2 commits February 3, 2020 23:35

feat: 🎸 catch up with v14

be69035

fix: 🐛 better serialization handling in useLocalStorage hook

68fb835

streamich approved these changes Feb 3, 2020

View reviewed changes

streamich merged commit 5bfdf65 into streamich:v14.0 Feb 3, 2020

nspaeth mentioned this pull request Mar 5, 2020

fix(#785): update useStorage to use createGlobalState #1025

Closed

12 tasks

streamich added the released label May 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix use local storage #786

Fix use local storage #786

TylerR909 commented Nov 20, 2019

TylerR909 commented Nov 20, 2019 •

edited

TylerR909 Nov 20, 2019

ayush987goyal commented Jan 16, 2020

mayteio commented Jan 30, 2020

TylerR909 commented Jan 31, 2020

roninjin10 Feb 1, 2020

TylerR909 Feb 1, 2020

roninjin10 Feb 2, 2020

roninjin10 Feb 1, 2020

TylerR909 Feb 1, 2020

roninjin10 Feb 2, 2020

roninjin10 commented Feb 1, 2020

TylerR909 commented Feb 2, 2020

roninjin10 commented Feb 2, 2020

TylerR909 commented Feb 3, 2020

streamich left a comment •

edited

TylerR909 commented Feb 4, 2020

streamich commented May 16, 2020

Fix use local storage #786

Fix use local storage #786

Conversation

TylerR909 commented Nov 20, 2019

Description

Type of change

Checklist

TylerR909 commented Nov 20, 2019 • edited

TylerR909 Nov 20, 2019

Choose a reason for hiding this comment

ayush987goyal commented Jan 16, 2020

mayteio commented Jan 30, 2020

TylerR909 commented Jan 31, 2020

roninjin10 Feb 1, 2020

Choose a reason for hiding this comment

TylerR909 Feb 1, 2020

Choose a reason for hiding this comment

roninjin10 Feb 2, 2020

Choose a reason for hiding this comment

roninjin10 Feb 1, 2020

Choose a reason for hiding this comment

TylerR909 Feb 1, 2020

Choose a reason for hiding this comment

roninjin10 Feb 2, 2020

Choose a reason for hiding this comment

roninjin10 commented Feb 1, 2020

TylerR909 commented Feb 2, 2020

roninjin10 commented Feb 2, 2020

TylerR909 commented Feb 3, 2020

streamich left a comment • edited

Choose a reason for hiding this comment

TylerR909 commented Feb 4, 2020

streamich commented May 16, 2020

TylerR909 commented Nov 20, 2019 •

edited

streamich left a comment •

edited