fix(useAsyncStorage): make more robust #760
Conversation
|
The current jest tests in the example folder don't seem to work. I'd suggest to treat the example app as its own project with own package file instead. The libs dependency should not carry an dependencies/tests the example project needs. |
|
Hey @pke, do you need help with anything? |
pke
force-pushed
the
fix/useAsyncStorage
branch
2 times, most recently
from
0cda315 to
c0a06f6
Compare
|
Updated. Removed code not required for this PR. Note: The test command does not run successfully locally, it shows many TS linting errors in the App/example tests. |
pke
force-pushed
the
fix/useAsyncStorage
branch
from
c0a06f6 to
b75dd11
Compare
src/hooks.ts
Outdated
| ); | ||
|
|
||
| const setItem = React.useCallback( | ||
| //@ts-ignore |
There was a problem hiding this comment.
We shouldn't add any @ts-ignore. What's the error here? Can we fix it?
There was a problem hiding this comment.
Yes its required because TS complains:
[ts] src/hooks.ts(11,44): error TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.
src/hooks.ts
Outdated
|
|
||
| const mergeItem = React.useCallback( | ||
| (...args) => | ||
| //@ts-ignore |
There was a problem hiding this comment.
We shouldn't add any @ts-ignore. What's the error here? Can we fix it?
| const setItem = React.useCallback( | ||
| //@ts-ignore | ||
| (...args) => AsyncStorage.setItem(key, ...args), | ||
| [key] |
There was a problem hiding this comment.
Do we have to create a new array for each of the callbacks? Can we create it once and reuse?
There was a problem hiding this comment.
No, this doesn't work. Eslint will complain:
const dep = [key]
const getItem = React.useCallback(
(...args) => AsyncStorage.getItem(key, ...args),
dep
);const dep: string[]
React Hook React.useCallback was passed a dependency list that is not an array literal. This means we can't statically verify whether you've passed the correct dependencies.eslint[react-hooks/exhaustive-deps](https://github.com/facebook/react/issues/14920)
React Hook React.useCallback has a missing dependency: 'key'. Either include it or remove the dependency array.eslint[react-hooks/exhaustive-deps](https://github.com/facebook/react/issues/14920)
__tests__/useAsyncStorage.test.ts
Outdated
| /** | ||
| * @format | ||
| */ | ||
| /* eslint-disable no-shadow */ |
There was a problem hiding this comment.
Why is this ESLint disable necessary?
|
After doing some more jest testing in my own project I came to the conclusion that the RNAS tests are somewhat broken. Maybe the mock is broken. I need to investigate more. |
|
I have not much time to work on that further, since I also moved to an alternative sync lib for settings. The tests for the hooks don't seem to run, not sure whats wrong with the setup there. Complaining about not able to find the RNAS mock. |
|
This PR has been marked as stale due to inactivity. Please address any comments within 7 days or it will be closed. |
Summary
Fixes the broken
useAsyncStorageand introduces a newuseAsyncStorageValueas a true hook replacement.I think we should not change the signature of useAsyncStorage for now in a patch release.
In this patch release we could also already introduce the new
useAsyncStorageValuewhich will be renamed touseAsyncStoragein the next major version.I'll also add
watchsupport to make the hook truly magic and work across multiple components.I'll first add the watch support to the hooks infrastructure only, that means any changes done by AsyncStorage.setXXX/mergeXXX are not detected. But when one only uses the hooks, then changes in values are propagated into each hook.
Test Plan
yarn jest __tests___for now.