-
Notifications
You must be signed in to change notification settings - Fork 31
fix: app crashing due to undefined settings constant accessor #923
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
base: main
Are you sure you want to change the base?
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
|
@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
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.
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.
| ios: () => { | ||
| const settings = | ||
| NativeModules.SettingsManager?.settings ?? | ||
| NativeModules.SettingsManager?.getConstants()?.settings; |
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.
Bug: Incorrect accessor path for getConstants() in Fabric
The fallback accessor NativeModules.SettingsManager?.getConstants()?.settings likely has an incorrect property path. In React Native's new Fabric architecture, getConstants() typically returns constants directly at the top level (e.g., {AppleLocale: 'en_US'}), not nested under a settings property. This means getConstants()?.settings would return undefined, causing AppleLocale to never be found even when available through getConstants(). The accessor likely needs to be getConstants()?.AppleLocale or getConstants() should replace the entire settings variable.
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 :)
Note
Fixes iOS locale detection to avoid crashes on Fabric-enabled React Native by falling back to SettingsManager.getConstants; also adds a lint:fix script.
src/platform/locale.ts): UsePlatform.selectand fall back toSettingsManager.getConstants()?.settingsfor iOSAppleLocale; keep Android usingI18nManager.localeIdentifier.lint:fixscript inpackage.json.Written by Cursor Bugbot for commit 3d8605d. This will update automatically on new commits. Configure here.