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

Allow consumption of RCTAsyncStorage as TurboModule #965

Merged
merged 2 commits into from
Jun 6, 2023

Conversation

rozele
Copy link
Contributor

@rozele rozele commented May 26, 2023

Summary

In react-native-windows, attributed C++ modules can be consumed as either a legacy native module or TurboModule. This change enables registration of RNCAsyncStorage as a TurboModule.

Test Plan

Tested this out in a react-native-windows C++ app, where instead of using the ReactPackageProvider, I register RNCAsyncStorage.h directly as a TurboModule.

In react-native-windows, attributed C++ modules can be consumed as either a legacy native module or TurboModule. This change enables registration of RNCAsyncStorage as a TurboModule.
// TurboModuleRegistry falls back to NativeModules so we don't have to try go
// assign NativeModules' counterparts if TurboModuleRegistry would resolve
// with undefined.
let RCTAsyncStorage = TurboModuleRegistry
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just bump the minimum requirement to 0.61 so we don't have to do this check. What do you think @krizzu?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the change, although that means we'd have to keep a compatibility table in readme (so that, we show which version is supported by which RN version). I guess it's safe to say we support 0.60 and up version

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's do this after this PR merges. Can you fix the Release job? It looks like it's unable to fetch user and password for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

@tido64 sorry, missed the message
I created PR to separate release from Pull Request checks:
#972

The PRs from external contributor fails, because of nature of "pull_request" trigger preventing access to Secrets and limiting GH token to READ only

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't realize this was an issue. Thanks for looking into it. I guess that means we can merge this now?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's get this in 🙏

@tido64 tido64 merged commit 62732d9 into react-native-async-storage:master Jun 6, 2023
@AsyncStorageBot
Copy link
Collaborator

🎉 This PR is included in version 1.18.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants