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

core asyncstorage and community version #118

Closed
rogueturnip opened this issue Jun 6, 2019 · 27 comments
Closed

core asyncstorage and community version #118

rogueturnip opened this issue Jun 6, 2019 · 27 comments

Comments

@rogueturnip
Copy link

I'm hoping someone could answer my question about a problem I'm seeing.

I'm using aws-amplify which is still using the old react-native core version of asyncstorage. I've moved my app over to using the react-native-community version. When I look at the storage while the app is running both are populating the asyncstorage correctly but when I restart the app only the data from my app using the react-native-community version is still there.

I've tried many things to try to fix this but to no luck. What I'm wondering is if there is a chance of conflict between the two if a module is still using the old version.

Thanks!

@krizzu
Copy link
Member

krizzu commented Jun 6, 2019

Hi @rogueturnip,

Thanks for bringing this up. Could you provide a repo where the issue is reproducible? Would be helpful in further debugging.

Also, what platform are you seeing this?

thanks

@rogueturnip
Copy link
Author

Sorry I don't actually have this in a public repo. The app is private so I can't open it up. The problem only occurs with ios and only with the aws amplify asyncstorage. Android everything works fine.I

Tomorrow I will try adding in the old asyncstorage in my code to add something and see if I get the same issue.

@rogueturnip
Copy link
Author

I did a simple test this morning. In my main app code I set up the old AsyncStorage from react-native core along with the new AsyncStorage from react-community. The core one did not persist through an IOS app restart but the react-community one did.

I don't have a repo for you to see this but it's a very simple set up, 2 lines of code with each writing something different to asyncstorage. Still no issues on Android.

@krizzu
Copy link
Member

krizzu commented Jun 6, 2019

@rogueturnip right. Can you provide some code samples? It's important how you call those two "writings" to AS.

thanks

@rogueturnip
Copy link
Author

Hi!

So obviously my code has a bunch more in it but basically I'm calling it like below. My real concern is that is if there is a special way to make these work correctly together that the library "aws-amplify" which uses the old asyncstorage won't work right.

Thanks!

import React, { Component } from 'react';
import { AsyncStorage as oldAS } from 'react-native';
import AsyncStorage from '@react-native-community/async-storage';

class TestApp extends Component {
    constructor(props) {
        super(props);
        oldAS.setItem('oldtest','testing old');
        AsyncStorage.setItem('newtest', 'testing new');
    }

    render() {
        return null;
    }
}

@krizzu
Copy link
Member

krizzu commented Jun 7, 2019

@rogueturnip .setItem is a asynchronous function. Without waiting for it to finish, you expose yourself for unexpected behavior (such as one you have now).

Try out making those calls wait for results, before going further.

class TestApp extends Component {
    constructor(props) {
        super(props);
		this.saveIt()
    }

	saveIt = async () => {
	    await oldAS.setItem('oldtest','testing old');
        await AsyncStorage.setItem('newtest', 'testing new');
	}
    render() {
        return null;
    }
}

@rogueturnip
Copy link
Author

Thanks for that. I tried it with await and it's giving me the same results on ios.

@krizzu
Copy link
Member

krizzu commented Jun 8, 2019

@rogueturnip you mean it's still an issue for you?

@rogueturnip
Copy link
Author

Yes, I'm still having the original AsyncStorage not saving across reloads of the iOS app. it's only iOS. I wanted to point that out in case it's something related to the linking that's causing the issue.

@krizzu
Copy link
Member

krizzu commented Jun 8, 2019

The implementation of Async Storage (both Comms and core one) for iOS is based on writing/reading from a file. This is the same file for both versions (so during the transition from the core to Community's AS, you won't lose data).

There's this comment left from original authors. This tiny detail is the key to understand the problem.

Each time your app restarts (or you reload the bundle), storage's file is read to the _manifest variable. Each data save/read (setItem/getItem) is done on the _manifest, but once the mutation has finished, its content is saved to the file.

This is a condition race. The last call to setItem will dump the content to the storage file, overriding what another previous one has saved.

My advice here:

  • If your dependencies do not use Async Storage, use Community's
  • If you're not able to control your dependencies (you cannot change which Async Storage version they use), use Core's Async Storage. Otherwise, swap their version of AS to Community.

There's Async Storage v2 coming, with a different approach for storage capabilities, making it more transparent and less error-prone in the future. Please look forward to it.

thanks.

@rogueturnip
Copy link
Author

Brilliant! Thanks so much. I thought this might be an issue at the lower levels. I'm going to post this over to the AWS Amplify team so they can see the potential issue and then go back to using just core.

Do you think it's safe to leave the community version linked so it's ready to go later?

@krizzu
Copy link
Member

krizzu commented Jun 8, 2019

@rogueturnip Yeah, it would be fine.

Please close the issue if you think that the case is solved.

thanks.

@rogueturnip
Copy link
Author

I don't have my iOS environment available until monday (unless I get time to go in and use it) so I'll test on Monday and close then.

Thanks again for your help.

@rogueturnip
Copy link
Author

Looks all good using the old core. Thanks!

@willdady
Copy link

@krizzu the link in your comment is returning a 404.

@mllrmat
Copy link

mllrmat commented Jul 29, 2019

Is there any strategy to avoid this behavior? I have the following problem: one of my dependencies is using the core async storage (aws amplify), another the community version (react-native-permissions). In my code I can decide which to use, but thats impossible for dependencies. How can I ensure consistency?

@krizzu
Copy link
Member

krizzu commented Jul 29, 2019

@mllrmat I can think of using babel module resolve plugin for that.

@mllrmat
Copy link

mllrmat commented Jul 30, 2019

@krizzu what would the config look like if I want to replace the core module with the community one? I tried this:

{
  "plugins": [
    [
      "module-resolver",
      {
        "alias": {
          "react-native/AsyncStorage": "@react-native-community/async-storage"
        }
      }
    ]
  ]
}

But it seams to have no effect.

@krizzu
Copy link
Member

krizzu commented Jul 30, 2019

@mllrmat It's because you're not using react-native/AsyncStorage to import it.
The problem here is that AsyncStorage from the community version is a default import, while from RN Core its named.

@danrivett
Copy link

danrivett commented Aug 12, 2019

Maybe I'm just being very naive - very likely as I'm a very green React developer - but I've struggled for about 5 hours debugging the AWS Amplify library trying to figure out why my logged-in state was not being persisted, and turns out it's not a bug with AWS Amplify or my code, it's because of this incompatibility between the deprecated AsyncStorage and the new one recommended to us, and I didn't read any warnings indicating this (https://facebook.github.io/react-native/docs/asyncstorage just says use this community version instead, seems pretty definitive).

This is deeply frustrating as I don't think it's reasonable for React developers to know all implementation details of packages they depend on and so know which version of AsyncStorage to use - I just used the new one recommended in the official React Native docs and got seriously burnt.

Is there a flag in the new community AsyncStorage version to write to a different file and so avoid this race condition? If there is then I could avoid having to tightly couple my AsyncStorage version I'm using to the one used by one of the libraries I'm using. Plus as @mllrmat points out, if competing dependencies use different AsyncStorage versions life becomes even more complicated again.

Please advise. Perhaps I'm just being very naive and precious here, but after a weekend of debugging another library's code and finding the root cause to be this - which seems very avoidable and predictable - I'd like to suggest an improvement here and at least provide an option for both the deprecated and community versions to be able to play nicely together.

@krizzu krizzu added the LEGACY label Aug 12, 2019
@krizzu
Copy link
Member

krizzu commented Aug 12, 2019

Hey @danrivett ,

Due to React Native core getting slimmer, some packages that were previously bundled together with React Native were moved out to separate repos (AsyncStorage included). In order to give some transition time, the deprecation warning was used.

Some devs moved immediately to use AsyncStorage from Community org, while others decided to wait with that decision. For example, apmplify-js uses built-in version. This is a situation I mentioned in my comment above.

Unfortunately, we don't have the feature to change save destination file in place (I'm open for help with implementing this). I can safely say that you're good to use deprecated Async Storage, for now, just ignore the warning.

@taschik
Copy link

taschik commented Aug 22, 2019

@krizzu I have the same situation where we use aws' amplify-js and this uses the old one, that we absolutely need to revert back to 1.2.4 to navigate around inconsistency in persisting data?

I encountered the issue with v.1.6.1 that values are not reliably persisted. After restarts, the data is gone leading to quite some troubles within the app. My setup is:

@react-native-community/async-storage v1.6.1

info Fetching system and libraries information...
System:
    OS: macOS 10.14.6
    CPU: (12) x64 Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
    Memory: 139.04 MB / 32.00 GB
    Shell: 5.3 - /bin/zsh
  Binaries:
    Node: 10.16.0 - /usr/local/bin/node
    Yarn: 1.17.3 - /usr/local/bin/yarn
    npm: 6.9.0 - /usr/local/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  SDKs:
    iOS SDK:
      Platforms: iOS 12.4, macOS 10.14, tvOS 12.4, watchOS 5.3
    Android SDK:
      API Levels: 23, 26, 27, 28
      Build Tools: 23.0.1, 25.0.0, 26.0.3, 27.0.3, 28.0.1, 28.0.2, 28.0.3
      System Images: android-24 | Google APIs Intel x86 Atom, android-24 | Google Play Intel x86 Atom, android-27 | Google APIs Intel x86 Atom, android-28 | Google APIs Intel x86 Atom, android-29 | Google APIs Intel x86 Atom
  IDEs:
    Android Studio: 3.4 AI-183.6156.11.34.5692245
    Xcode: 10.3/10G8 - /usr/bin/xcodebuild
  npmPackages:
    react: ^16.8.6 => 16.8.6
    react-native: ^0.60.5 => 0.60.5
  npmGlobalPackages:
    eslint-plugin-react-native: 3.5.0
    react-native-cli: 2.0.1
    react-native-git-upgrade: 0.2.7

@krizzu
Copy link
Member

krizzu commented Aug 22, 2019

@taschik
there's no need to revert back to 1.2.4 - you can simply drop usage of Community's
AsyncStorage.

@taschik
Copy link

taschik commented Aug 22, 2019

@krizzu How do you mean this? Remove the package completely?

@krizzu
Copy link
Member

krizzu commented Aug 22, 2019

@taschik If you can't change what version of Async Storage your dependencies are using, I advise to not use this package for now - to avoid race conditions and possible loss of data.

@taschik
Copy link

taschik commented Aug 22, 2019

ah get it ok. Thanks a lot I reverted to importing async-storage from react-native and removed the package at all. Thank you so much for the support. I am super excited to see async-storage 2.0 coming along. Good work guys!

@stephenheron
Copy link

Yeah we just ran into this issue and it was a tough one to track down as it was inconsistent behaviour. Should a warning be added to the readme around this as it is not obvious that you can't mix and match.

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

No branches or pull requests

7 participants