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

Add Windows AsyncStorage #327

Merged
merged 8 commits into from
Apr 27, 2020

Conversation

kaiguo
Copy link
Contributor

@kaiguo kaiguo commented Apr 6, 2020

Summary:

Add Windows AsyncStorage.
Closes microsoft/react-native-windows#2271.

Test Plan:

  1. System requirments.
  2. Run yarn start:windows.
  3. Open example/windows/AsyncStorageExample.sln with VisualStudio and run.

Screenshot_20200406162637

@kaiguo
Copy link
Contributor Author

kaiguo commented Apr 7, 2020

@krizzu there are bunch of Flow check errors inside react-native/Libraries from test pipeline, any idea how to address them?

https://circleci.com/gh/react-native-community/async-storage/2125?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

windows/ReactNativeAsyncStorage/RNCAsyncStorage.h Outdated Show resolved Hide resolved
windows/ReactNativeAsyncStorage/RNCAsyncStorage.h Outdated Show resolved Hide resolved
windows/ReactNativeAsyncStorage/DBStorage.cpp Outdated Show resolved Hide resolved
windows/ReactNativeAsyncStorage/DBStorage.cpp Outdated Show resolved Hide resolved
windows/ReactNativeAsyncStorage/DBStorage.cpp Show resolved Hide resolved
@krizzu
Copy link
Member

krizzu commented Apr 7, 2020

@kaiguo It's caused by bumping RN version, while Flow stays on the old one.
Is there a reason for changing it to 0.61? I haven't tested AsyncStorage with that version yet.

@kaiguo
Copy link
Contributor Author

kaiguo commented Apr 7, 2020

Is there a reason for changing it to 0.61? I haven't tested AsyncStorage with that version yet.

@krizzu react-native-windows has a peer dependency on RN 0.61.5. I guess I can revert it to 0.58 for now, the downside is windows sample won't compile/run😢.

@krizzu
Copy link
Member

krizzu commented Apr 8, 2020

@krizzu react-native-windows has a peer dependency on RN 0.61.5. I guess I can revert it to 0.58 for now, the downside is windows sample won't compile/run😢.

I see. Well, we just need to make sure that AS works with 0.61.5 now.
For RN 0.61.5, try upgrading to Flow version 0.105.0. Here's a config used in RN 0.61.5 - I guess we just need to do some adjustments.

@kaiguo
Copy link
Contributor Author

kaiguo commented Apr 10, 2020

@krizzu flowconfig changes did fix the flow check errors, thanks! But looks like both iOS and Android builds are broken after upgrading to 61. I created a separate branch here which fixed Android and Flow issues. For iOS, I think we need to switch to pod linking, looks like that's the only supported way on 61+. I'm not very familiar with iOS so I created #328 to track the rest of the work.

I'll keep react-native on 59 in this PR for now.

@kaiguo
Copy link
Contributor Author

kaiguo commented Apr 11, 2020

PR that upgrades iOS and Android sample apps to RN 61: #329.

@krizzu
Copy link
Member

krizzu commented Apr 12, 2020

@kaiguo Great, let's fix merge #328 first, so you can keep this PR with RN 61.

@krizzu krizzu changed the base branch from LEGACY to master April 12, 2020 17:05
@krizzu
Copy link
Member

krizzu commented Apr 22, 2020

@kaiguo Sorry for keeping you waiting, but I'm waiting for someone else to have a look here (with Windows experience, asked at RN channel).

@kaiguo
Copy link
Contributor Author

kaiguo commented Apr 22, 2020

@kaiguo Kai Guo FTE Sorry for keeping you waiting, but I'm waiting for someone else to have a look here (with Windows experience, asked at RN channel).

Sure, good to have some extra eyes on this : )

"@react-native-community/cli": "4.6.3",
"@react-native-community/cli-platform-ios": "4.6.3",
"@react-native-community/cli-platform-android": "4.6.3",
"@react-native-community/cli": "^3.1.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there issues with the the newer version of RNCLI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4.x has a peer dependency on RN 62, the test apps are on 61 so it generated bunch of warnings in yarn install. 3.x is probably a better match for RN 61.

@krizzu
Copy link
Member

krizzu commented Apr 27, 2020

I think there's no need to prolong this PR. Thanks for all the work @kaiguo 💪 🎉

@krizzu krizzu merged commit 3dd9e0e into react-native-async-storage:master Apr 27, 2020
@kaiguo kaiguo deleted the add-windows-legacy branch April 27, 2020 18:16
@kaiguo
Copy link
Contributor Author

kaiguo commented Apr 28, 2020

@krizzu thanks for merging this in.

I'm trying to use the module in another project and just noticed the changes haven't been published yet, do we know when the next release will be?

@krizzu
Copy link
Member

krizzu commented Apr 28, 2020

@kaiguo I'm going to issue new release later today

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 this pull request may close these issues.

Add support for Community's Async Storage
4 participants