useAsyncStorage is NOT a HOOK

[seancheung]

Current behavior

According to hooks.js, useAsyncStorage is a wrapper rather than a hook.

// return a new object at every call
export function useAsyncStorage(key: string): AsyncStorageHook {
  return {
    getItem: (...args) => AsyncStorage.getItem(key, ...args), // assign a new prop at every call
    setItem: (...args) => AsyncStorage.setItem(key, ...args),
    mergeItem: (...args) => AsyncStorage.mergeItem(key, ...args),
    removeItem: (...args) => AsyncStorage.removeItem(key, ...args),
  };
}

It will either trigger unexpected re-rendring:

const MyComponent = () => {
  const wrapper = useAsyncStorage('mykey');

  React.useEffect(() => {
    wrapper.getItem().then(res => {
      //...
    })
  }, [wrapper]); // <------ wrapper changes at every re-rendering
}

const MyComponent = () => {
  const { getItem } = useAsyncStorage('mykey');

  React.useEffect(() => {
    getItem().then(res => {
      //...
    })
  }, [getItem]); // <------ getItem changes at every re-rendering
}

Or eslint hooks warnings:

const MyComponent = () => {
  const wrapper = useAsyncStorage('mykey');

  React.useEffect(() => {
    wrapper.getItem().then(res => {
      //...
    })
  }, []); // <------ react-hooks/exhaustive-deps warning
}

const wrapper = useAsyncStorage('mykey'); // <------ react-hooks/rules-of-hooks warning

const MyComponent = () => {
  React.useEffect(() => {
    wrapper.getItem().then(res => {
      //...
    })
  }, []);
}

A workaround is to disable warnings:

// eslint-disable-next-line react-hooks/rules-of-hooks
const wrapper = useAsyncStorage('mykey');

const MyComponent = () => {
  React.useEffect(() => {
    wrapper.getItem().then(res => {
      //...
    })
  }, []);
}

Expected behavior

• If useAsyncStorage is not a standard hook, it should be renamed to something like withAsyncStorage rather than starting with use which will be treated as a hook.

export function withAsyncStorage(key: string): AsyncStorageHook {
  return {
    getItem: (...args) => AsyncStorage.getItem(key, ...args),
    setItem: (...args) => AsyncStorage.setItem(key, ...args),
    mergeItem: (...args) => AsyncStorage.mergeItem(key, ...args),
    removeItem: (...args) => AsyncStorage.removeItem(key, ...args),
  };
}

• If useAsyncStorage is designed to be a standard hook, the returned value should be memo-ed

export function useAsyncStorage(key: string): AsyncStorageHook {
  return React.useMemo(() => ({
    getItem: (...args) => AsyncStorage.getItem(key, ...args),
    setItem: (...args) => AsyncStorage.setItem(key, ...args),
    mergeItem: (...args) => AsyncStorage.mergeItem(key, ...args),
    removeItem: (...args) => AsyncStorage.removeItem(key, ...args),
  }), []);
}

• This should be documented

Repro steps

Environment

• Platforms tested:
  • Android
  • iOS
  • macOS
  • Windows
• AsyncStorage version:
• Expo version:
• Environment:
• Logs/Errors that may be relevant:

[krizzu]

Thanks @seancheung for feedback. The current implementation of useAsyncStorage is experimental, like we mention this in the documentation, so I'd not recommend using it in the current state.

[gustavopch]

It's extremely easy to fix it. It's really just a matter of memoizing the returned object. It's not like a lot of experimentation should be made until a good solution could be found. It has been brought up in multiple issues and just ignored. I understand maintainers may not have time to fix it themselves, but it's like not even a PR would be welcome despite being such a simple thing to solve that's rendering the hook unusable.

[pke]

And those bug in this "hook" are easy to spot in tests. The returned identities of the function must not change across re-renders.

@krizzu would you accept a PR?

But thinking about it, in the current state the hook is pretty much useless, because all it does is a (faulty) property binding.
What it lacks is proper notification when a value changes.
So the proper hook would be

function MyFeature() {
  const [value, setValue] = useAsyncStorage("myFeature", "default")

  return <Text>{value}</Text>
}

Whenever the myFeature is changed the hook triggers an update. Without the refusal to have global event listeners, this is pretty much no doable at the moment though.

[krizzu]

even a PR would be welcome despite being such a simple thing to solve that's rendering the hook unusable.

@gustavopch @pke
The initial implementation of the hook is not ideal, so I mentioned this many times to not use it. The main reason I think it's bad is the lack of synchronization with native side, so if something changes outside of hook, the returned is not up to date.
I believe this hook should, along side with the value and methods to modify/remove, return a state to reflect the state of value (is initial load from async storage done? etc.).

I'm happy to open a new discussion about getting this right this time 🙏

[pke]

I think a warning will not be required. I will back up the hook with a proper test suite 😉

[pke]

Ok, so it seems the jest tests included with the example app are not working? They also do not seem to be run by any test script in the package file. I wanted to establish a baseline but the error I am getting is:

 ● Test suite failed to run

    Cannot find module '@react-native-async-storage/async-storage' from 'example/jest.setup.js'

    > 1 | /**
        |              ^
      2 |  * @format
      3 |  */
      4 |

      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:306:11)
      at Object.<anonymous> (example/jest.setup.js:1:104)

which makes sense, since no such package is declared in the package file.

At least one of the tests seems to be wrong too, since you cannot call setItem with number as value. Hence the test as its written should fail if it would be able to run. And it does fail, as I can see on my new Test suite:

 useAsyncStorage › can read/write data to/from storage

    expect(received).toBe(expected) // Object.is equality

    Expected: 896
    Received: "896"

      34 |     const data = await result.current.getItem('key');
      35 |
    > 36 |     expect(data).toBe(newData);
         |                  ^
      37 |   });

[pke]

 PASS  __tests__/useAsyncStorage.test.ts
  useAsyncStorage
    ✓ should export only stable output (14 ms)
    ✓ can read/write data to/from storage (4 ms)
    ✓ can clear storage (2 ms)
    ✓ can clear entries in storage (5 ms)
    ✓ can use merge with current data in storage (7 ms)
  useAsyncStorageValue
    ✓ should export stable output (4 ms)

[pke]

I am getting all kind of weird testing behaviour now, since I introduced afterEach.
Nevermind this, the vscode autoimport had imported afterEach from detox ;)

However my second problem with only still stands.

DetoxRuntimeError: Detox instance has not been initialized

    HINT: Make sure to call detox.init() before your test begins

       7 | import AsyncStorage, { useAsyncStorage, useAsyncStorageValue } from '../src';
       8 |
    >  9 | afterEach(() => {
         | ^
      10 |   return AsyncStorage.clear();
      11 | });
      12 |

      at MissingDetox.throwError (node_modules/detox/src/utils/MissingDetox.js:68:11)
      at MissingDetox.<anonymous> (node_modules/detox/src/utils/MissingDetox.js:9:70)
      at <computed> (node_modules/detox/src/DetoxExportWrapper.js:94:32)
      at Object.<anonymous> (__tests__/useAsyncStorage.test.ts:9:1)

Also I can no longer do .only on tests it will completely break with weird import errors all of the sudden.

How do I prevent detox to hijack the tests in that way?

[pke]

I have created a preview of my PR #760

[seancheung]

This isn't a hook since the beginning as it has nothing to do with state. Users think of it as a hook because its name is confusing(A custom Hook is a JavaScript function whose name starts with ”use” and that may call other Hooks.).
I think the best approach is to rename useAsyncStorage to something hookless like withAsyncStorage. That would bring peace to the world.

const wrapper = withAsyncStorage('mykey');

const MyComponent = () => {
  React.useEffect(() => {
    wrapper.getItem().then(res => {
      //...
    })
  }, []);
}

The synchronization issue @krizzu worries about does not exist because this is just a closure which actually transforms doSomething(key) to (key) => () => doSomething(key) and nothing else(property binding like @pke said).

The state part is not something this library should care about(and I think the use case is very rare). It can be implemented as below

function useAsyncStorage(key) {
  const [value, setValue] = React.useState();
  React.useEffect(() => {
    AsyncStorage.getItem(key).then((value) => setValue(value));
  }, [key]);
  React.useEffect(
    () =>
      // TODO: add a listener function to AsyncStorage which allows us to be notified when the value changes,
      // and it must return a function to remove the listener
      AsyncStorage.on("change", key, (value) => {
        setValue(value);
      }),
    [key]
  );
  const setItem = React.useCallback(
    async (value) => {
      await AsyncStorage.setItem(key, value);
      setValue(value);
    },
    [key]
  );
  const removeItem = React.useCallback(async () => {
    await AsyncStorage.removeItem(key);
    setValue(undefined);
  }, [key]);
  // TODO: mergeItem
  const handler = React.useMemo(
    () => ({ setItem, removeItem }),
    [setItem, removeItem]
  );
  return [value, handler];
}

function App() {
  const [item, { setItem, removeItem }] = useAsyncStorage("key");
}

[pke]

Please check my PR #760 it also handles edge cases like unmounting while loading from storage.
It also never waits for any Async Storage operation to finish internally, because I don't think its necessary.

When the saving of the setting fails, what is the app supposed to do (or the user in that case)?

[seancheung]

It also never waits for any Async Storage operation to finish internally

This might cause synchronization problems.

When the saving of the setting fails, what is the app supposed to do

Imho it should crash. Your subscribers already get the changed value since you didn't wait for the saving operation to finish or fail. So your app is already in an unstable state.

For me there is no use case so I still think the state part is not something this library should care about (Why would anyone subscribe to an natively stored value? Its reactiveness will be redundant and confusing)

[pke]

Why would one subscribe to a settings change? Well because feature toggles and other use cases.

[blabno]

Alternative:

export function useAsyncStorage(key: string): AsyncStorageHook {
  return {
    getItem: useCallback((...args) => AsyncStorage.getItem(key, ...args), [key]),
    setItem: useCallback((...args) => AsyncStorage.setItem(key, ...args), [key]),
    mergeItem: useCallback((...args) => AsyncStorage.mergeItem(key, ...args), [key]),
    removeItem: useCallback((...args) => AsyncStorage.removeItem(key, ...args), [key])
  };
}

Just wrap those functions with useCallback.