-
Notifications
You must be signed in to change notification settings - Fork 361
fix: handle react-native-safe-area-context optionally for RN version >=0.81 #3239
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
fix: handle react-native-safe-area-context optionally for RN version >=0.81 #3239
Conversation
SDK Size
|
| import { getReactNativeVersion } from '../../../utils/getReactNativeVersion'; | ||
| import type { Photo } from '../ImageGallery'; | ||
|
|
||
| const SafeAreaView = getReactNativeVersion().minor >= 81 ? SafeAreaViewOriginal : RNSafeAreaView; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't check the version. Ideally, we should have this:
| const SafeAreaView = getReactNativeVersion().minor >= 81 ? SafeAreaViewOriginal : RNSafeAreaView; | |
| const SafeAreaView = RNSafeAreaView ?? SafeAreaViewOriginal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that on 0.81 its just deprecated and you get a warning that it will be removed in future versions. The integrators who are above or on 0.81 should not get the warning imo from the SDK atleast(we don't know which version will really remove the API) and the SafeAreaView in that case would still be the default RN's one. Imo we should utilize rn-safe-area-context regardless of the default API availability starting 0.81 so that integrators don't get a warning. Happy to change if you disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, shall we flip the condition? Use the external one by default, fall back to the native one if missing?
Would that yield a warning too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For <0.81 it won't be a problem(the native one will be used). For >=0.81, we are having external one as the default and the native one if external dep is missing. I think the change in the PR should cover all the cases unless I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it covers all cases, but what I don't think we should check against the actual RN version. Instead, let's make our code always use the external one, and fall back to RN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: if this is causing the deprecation warning to always trigger @khushal87 (for trying to import it from RN) you can always add it as an optional dependency for both Expo and RN CLI as packages. That way, this should not crash when we import it, regardless of us having a fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make our code always use the external one, and fall back to RN.
I have added the following change.
No description provided.