fix: app crashing due to undefined settings constant accessor #923
Conversation
- add check for undefined settings constant - add null coalescing operation to use getConstants to prevent undefined settings object
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| export default Platform.select({ | ||
| ios: getAppleLocale(), | ||
| android: NativeModules.I18nManager?.localeIdentifier, | ||
| }) |
There was a problem hiding this comment.
Bug: Platform Select Returns Undefined, Calls iOS Code Unnecessarily
The Platform.select implementation now returns undefined for non-iOS/Android platforms, removing the previous I18nManager?.localeIdentifier fallback. Additionally, getAppleLocale() is called immediately on module load for all platforms, causing iOS-specific native module code to execute unnecessarily.
|
@CalhamJNorthway Thank you for the PR! What was the crash you are seeing? While I think that we need an update to reliably be accessing the locale, I don't see anything that was doing anything other than forwarding along the undefined value. Settings was undefined, but the app locale as already being conditionally accessed from it.
Though there was a previous version where it looked like this: Which would have crashed. Thank you, |
Hey @kinyoklion, thanks for the prompt reply. I can add a little more detail, the issue was actually with the launch darkly client not instantiating correctly when the locale was undefined. You end up getting an error
Which after some further digging, I'm assuming was caused by the error below bubbling up to a higher error boundary
Also for posterity, I have validated this fix with a node module patch on our application. |
joker23
left a comment
joker23
left a comment
There was a problem hiding this comment.
Thanks @CalhamJNorthway for the PR! Sorry for the delay in response, but I think this PR would be ready to merge after addressing my comments.
| } | ||
|
|
||
| export default locale; | ||
| export default Platform.select({ |
There was a problem hiding this comment.
we should probably wrap the value assignments in a function so only the necessary values are being calculated, eg:
ios: () => getAppleLocale()
| export default locale; | ||
| export default Platform.select({ | ||
| ios: getAppleLocale(), | ||
| android: NativeModules.I18nManager?.localeIdentifier, |
There was a problem hiding this comment.
change case to default to preserve original fallback behavior
3 participants
The purpose of this PR is to fix a crash I found yesterday caused by an accessor that is undefined when newer versions of react native have Fabric enabled.
Our Build Setup:
react-native: v0.80.0@launchdarkly/react-native-client-sdk: v10.1.5Requirements
getConstantsto prevent undefinedsettingsobjectRelated issues
I've not seen any related issues yet regarding your repo, though I did find this one Stack Overflow post.
Describe the solution you've provided
The solution I've provided is to use a null coalescing check to fallback to the new
getConstantsmethod on native modules if thesettingsobject is undefined.Describe alternatives you've considered
I searched to see if there are preferred alternatives and did not come up with any other solutions. I am open to feedback on that though :)