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 compatibility with Expo SDK 52 #940

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adam-sajko
Copy link

@adam-sajko adam-sajko commented Nov 12, 2024

Summary

This PR updates RNDateTimePickerPackage for compatibility with Expo SDK 52. It modifies createNativeModules to match Expo's recommended patterns and adjusts ReactModuleInfoProvider to improve dynamic handling of module information.

Expo PR: link

Test Plan

Prerequisites for Testing

  1. Expo SDK 52 environment.
  2. Simulator and physical device for testing compatibility.

Steps to Test

  1. Open the app in Expo Go.
  2. Navigate to the components that use RNDateTimePicker.
  3. Ensure the picker functions as expected on both iOS and Android.

Compatibility

OS Implemented
iOS
Android

Checklist

  • Tested on device and simulator
  • Updated relevant documentation (README.md)
  • Updated TypeScript definitions if applicable
  • Added a usage example in the example project (example/App.js)
  • Added relevant automated tests

Copy link
Member

@vonovak vonovak left a comment

Choose a reason for hiding this comment

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

Hello and thanks for the PR!

It modifies createNativeModules to match Expo's recommended patterns

Can you please link to the source of this? I'm not aware of these recommended patterns.

In fact, I prefer the old version of the file; there should be one way to create instances of the native modules: getModule, not 2 ways (getModule as well as createNativeModules); see https://reactnative.dev/docs/next/turbo-native-modules-android

Unless there's some other good reason to merge, I'd prefer to keep this as-is.

Thank you!

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.

2 participants