-
Notifications
You must be signed in to change notification settings - Fork 31
refactor: added warning if async storage is not found #1044
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?
Conversation
- Added a warning message for when async storage is not found and is being mocked. - Added tests to verify behavior with custom and default storage implementations.
|
@launchdarkly/browser size report |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
| return require('@react-native-async-storage/async-storage').default; | ||
| } catch (e) { | ||
| // Use a mock if async-storage is unavailable | ||
| logger.warn('AsyncStorage is not available, using a mock implementation.'); |
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.
I think we may want to consider not using mock. It doesn't convey what will actually happen.
I think we want to do a few things:
- Make it clear that there will be no persistence of generated keys or context cache.
- Make it easy to understand what is required to fix it.
I am also concerned implementation wise. It seems like even without storage we should be keeping things in memory to be stable for the application session. (This is a pre-existing problem we could have in a different PR.)
Also, will these be called even if RNStorage is specified? I feel like it probably would be, but wouldn't matter in that case.
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.
Ah yea, it would be good to be able to fallback to inmemory context storage. I can create a work item for that.
If a custom storage is provided, then this line should not run
| storage: storage ?? new PlatformStorage(logger), |
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 the message wording, how about:
AsyncStorage is not available, generate keys and context caches will not be persisted. Please see https://launchdarkly.github.io/js-core/packages/sdk/react-native/docs/interfaces/LDOptions.html#storage for more information.
Additionally, I think we can put more details in the documentation with links to the default implementation for https://github.com/react-native-async-storage/async-storage
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.
Sounds good.
generate -> generated
Requirements
Related issues
sdk-1676
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Note
Introduces a logger-aware AsyncStorage initialization with a fallback and verifies storage selection behavior.
ConditionalAsyncStorageintogetAsyncStorage(logger)that logs a warning and returns a mock when@react-native-async-storage/async-storageis unavailablePlatformStorageto initialize viagetAsyncStorageand proxyget/set/clearto the resolved instanceReactNativeLDClient.storage.test.tsto assert use of customstorage, absence of warnings, and defaultPlatformStorageinstantiation and method calls when no custom storage is providedEventSource.tsWritten by Cursor Bugbot for commit 3bc8c25. This will update automatically on new commits. Configure here.