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

atomWithStorage in React Native trouble using AsyncStorage #568

Closed
austinried opened this issue Jun 28, 2021 · 9 comments · Fixed by #569
Closed

atomWithStorage in React Native trouble using AsyncStorage #568

austinried opened this issue Jun 28, 2021 · 9 comments · Fixed by #569

Comments

@austinried
Copy link

Hi, not sure if I've got something configured wrong or what but it seems like setting up the atomWithStorage function is not working great for me in React Native. I started by defining a simple atom with storage and a default state:

export const appSettingsAtom = atomWithStorage<AppSettings>('@appSettings', {
  servers: [],
});

but whenever I try to set it I get this error:
image

After scanning through the code for atomWithStorage it looks like there is no AsyncStorage implementation at all currently. Turns out I didn't read the docs closely enough and had assumed that AsyncStorage support was built-in, but I guess it's not. Doesn't seem like I can pass AsyncStorage directly as the third param to atomWithStorage since it's not type compatible, so I tried to implement my own Storage object (which I would prefer to do anyway so I can handle my own logging later):

export async function getItem(key: string): Promise<any | null> {
  try {
    const item = await AsyncStorage.getItem(key);
    return item ? JSON.parse(item) : null;
  } catch (e) {
    console.error(`getItem error (key: ${key})`, e);
    return null;
  }
}

export async function setItem(key: string, item: any): Promise<void> {
  try {
    await AsyncStorage.setItem(key, JSON.stringify(item));
  } catch (e) {
    console.error(`setItem error (key: ${key})`, e);
  }
}

export const atomStore = {
  getItem,
  setItem,
  delayInit: true,
}

...but now I get an error about the value of the atom being null:
image

Ok, maybe that's because my getItem function can return null if the key doesn't exist, but that's weird since that's the default behavior for AsyncStorage. Alright, so now I re-implement getItem to also return the default value:

import { getItem, setItem } from '../storage/asyncstorage';

export const appSettingsAtom = atomWithStorage<AppSettings>('@appSettings', {
  servers: [],
}, {
  getItem: async () => {
    const item = await getItem('@appSettings');
    return item || { servers: [] };
  },
  setItem: setItem,
  delayInit: true,
});

And now it finally seems to work, but I have to tell it about my key and default value multiple times, which doesn't feel great. I could wrap this up in another utility function on my end that provides these things, but I guess I'm left wondering is this the intended workflow here with AsyncStorage or am I missing a built-in utility somewhere that simplifies this?

@dai-shi
Copy link
Member

dai-shi commented Jun 28, 2021

Hi! It's nice to see someone try atomWithStorage with RN's AsyncStorage.
We really wanted feedback on this.

const appSettingsAtom = atomWithStorage('@appSettings', {
  servers: [],
}, {
  getItem: (key) => AsyncStorage.getItem(key).then((str) => JSON.parse(str)),
  setItem: (key, value) => AsyncStorage.setItem(key, JSON.stringify(value)),
  delayInit: true,
});

I was hoping something like this would work. Would you please try it?
(Hm, maybe this doesn't handle async errors correctly?)

@austinried
Copy link
Author

Hey no problem, I'm switching from recoil in a prototype I'm working on currently and loving jotai so far, just got a little hung up on this part.

Your code is close but it doesn't quite compile for me with typescript due to str possibly being null (string | null) and JSON.parse() not handling null values. Additionally, I would want it to return the default value on the first get since the key will always not exist at first in storage before it's saved, so the closest to working I think I can get that example is something like this:

const appSettingsDefault = { servers: [] };

const appSettingsAtom = atomWithStorage('@appSettings', appSettingsDefault, {
  getItem: (key) => AsyncStorage.getItem(key).then((str) => str ? JSON.parse(str) : appSettingsDefault),
  setItem: (key, value) => AsyncStorage.setItem(key, JSON.stringify(value)),
  delayInit: true,
});

@dai-shi
Copy link
Member

dai-shi commented Jun 28, 2021

Thanks for the suggestion.

I made a small fix #569.

My hope is to work this:

const anyAsyncStorage = {
  getItem: (key) => AsyncStorage.getItem(key).then((str) => JSON.parse(str || '')),
  setItem: (key, value) => AsyncStorage.setItem(key, JSON.stringify(value)),
  delayInit: true,
}

const appSettingsAtom = atomWithStorage('@appSettings', appSettingsDefault, anyAsyncStorage)

Would you try the codesandbox build?
https://ci.codesandbox.io/status/pmndrs/jotai/pr/569 See "Local install instructions"

@austinried
Copy link
Author

Yep looks like PR build works for me with your example above, without it I get an unhandled promise rejection from JSON.parse():

[Info] 06-28 16:03:55.152 31501 31605 W ReactNativeJS: Possible Unhandled Promise Rejection (id: 1):
06-28 16:03:55.152 31501 31605 W ReactNativeJS: SyntaxError: JSON Parse error: Unexpected end of input

But after applying your change that is resolved.

I wonder if there could be a simpler way of integrating AsyncStorage in the future though without having to write that boilerplate? If you'd be open to it, I may try my hand at an implementation that does something like take a peer dependency on AsyncStorage an implements it directly as the storage when localStorage does not exist.

@dai-shi
Copy link
Member

dai-shi commented Jun 28, 2021

Cool. Thanks for confirming.

I'm not sure if those 5 lines is too much boilerplate, but my point is it's hard to fulfill everyone's need.
People want not only localStorage, but also sessionStorage, IndexedDB, and so on.

I wouldn't recommend the default localStorage either. They come with anys.
For better TS support, it would be better to have type guards, instead of bare JSON.parse.
And, with type guards, it's not boilerplate any more, but valid business logic.

That said, we are open for improvements for better developer experience.
I'm not sure if a new proposal would fit in official jotai/utils bundle,
of it might be time for us to create a new standalone third-party (or official) jotai-storage package.


Hm, I might have a small idea to expose json storage creator, which might make your use case easier. Let me see.

@dai-shi
Copy link
Member

dai-shi commented Jun 28, 2021

Check the latest code in the PR.

Now, it would allow you this:

const appSettingsAtom = atomWithStorage('@appSettings', appSettingsDefault, createJSONStorage(() => AsyncStorage))

Please git it a try.

@austinried
Copy link
Author

Gotcha, yeah we're going to have to deal with any either way when it comes to localStorage or AsyncStorage via JSON.parse(), or unknown with type guards.

I've tried your example above and it works great, although in TS if I want to keep the generic type of <AppSettings> so that my atom has a nice type I can work with in components/derived atoms, it seems I need to either add the as any for createJSONStorage() (which makes it pretty explicit that I'm trusting the strings in that store to be the type I say it is) or implement my own Storage<AppSettings> that makes use of createJSONStorage() but puts type checks and maybe throws an error during getItem(). Either way though I think that's good, it makes it easier for the most basic use case to get started with AsyncStorage as well.

I would suggest also that it might be good to export the Storage<T> type to make it easier for developers to implement those type-guarded (or totally custom) storage backends.

@dai-shi
Copy link
Member

dai-shi commented Jun 28, 2021

Thanks for confirming. Would you say the PR is ready for merge?

I would suggest also that it might be good to export the Storage<T> type to make it easier for developers to implement those type-guarded (or totally custom) storage backends.

Good point. However, I'm hesitant to export it at this point. It's not 100% fixed, and we should get more feedbacks before the fix. Furthermore, the name seems too general.
I wish we could extract the type from atomWithStorage, but TS doesn't allow extracting generics types? 😕

@austinried
Copy link
Author

Seems good to me, maybe exporting the Storage<T> type then is something that will be better for a more fully-fledged jotai-storage package like you mentioned in the future.

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 a pull request may close this issue.

2 participants