Allow configuring a custom event store in React Native tracker (closes #1413) #1418
Conversation
snowplowcla
added
the
cla:yes
There was a problem hiding this comment.
Thank you very much for this PR @valeriobelli!
I think this would be really useful, but the main problem is that it is a breaking change and would require us to release a new major version of the JS trackers (v5), which we didn't plan for now and we try to do sparingly.
Do you see a way that we could make this a non-breaking change? That means keep the same behaviour by default as the tracker has now (defaulting to async storage) but having an option to use something else. We probably can't get rid of the async-storage dependency for now, but we can provide a way for users to override it optionally and use something else.
valeriobelli
force-pushed
the
master
branch
2 times, most recently
from
88d76f1 to
dae08b3
Compare
|
Thanks for the prompt review, @matus-tomlein! I agree. My approach was too drastic. 😁 I've also had the chance to define a workspace setting to allow contributors to be aligned on the formatter. Let me know how it sounds; I can also remove it to reduce the impact of this PR. |
There was a problem hiding this comment.
Thanks for the changes @valeriobelli, this looks great!
I'm happy to get this out in a 4.5 release soon but I don't think we should move the async-storage dependency to peer dependencies, that would break existing installations for users which shouldn't happen in a minor release. Could you please keep it as a normal dependency?
.vscode/settings.json
Outdated
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
This can be useful, but please lets remove it from this PR as it is unrelated to the change done here.
| @@ -43,6 +43,7 @@ | |||
| "test": "jest" | |||
| }, | |||
| "peerDependencies": { | |||
| "@react-native-async-storage/async-storage": "~2.0.0", | |||
There was a problem hiding this comment.
I understand that it makes more sense to set async-storage as a peer dependency now so that it can more easily be ignored but unfortunately that'd be a breaking change for users who have previously relied on it being included in the tracker package (in the react-native-get-random-values case we did move it to peer dependencies in a minor version but that was because it had to be installed as a peer dependency anyway in the app so users probably had it installed like that anyway).
Could you please keep it as a dependency? Otherwise this will block us from releasing the change in the v4 tracker. Happy to move it to peer dependencies in v5 but that will take some time for us to get to.
There was a problem hiding this comment.
Sure, I can move it. 😊
Before proceeding, I have a little bit of doubt. I agree that moving it to the peerDependencies force this Pull Request to introduce a breaking change. However, doesn't it apply the same way for crypto.getRandomValues and react-native-get-random-values? Currently, in 4.4.0 and below, consumers are not notified to install @react-native-async-storage/async-storage (e.g., Yarn notifies consumers when a peer dependency is not correctly satisfied). They likely get a runtime error when the Snowplow tracker tries to access the global AsyncStorage, given there might not be available platform bindings, the same that happens when the tracker tries to access crypto.getRandomValues.
There was a problem hiding this comment.
Sorry for the delay on this.
Regarding the react-native-get-random-values case, I was under the impression that even though it was not a peer dependency, the tracker would not work without it being explicitly included in the app dependencies – that's what issue #1409 reported. I am not aware that the async-storage also needs to explicitly included in the app. At least based on my tests I didn't need to do so, but could be wrong?
There was a problem hiding this comment.
Hey @matus-tomlein, sorry for being late here. 🙇🏽
As far as I can see, React Native AsyncStorage must be linked, given it exposes React Native bindings.
In other words, the app will crash if a consumer omits @react-native-async-storage/async-storage in the package.json. This is a small repro.
The following is a preview of what would happen when omitted
There was a problem hiding this comment.
Okay, thank you for that reproduction!
There was a problem hiding this comment.
Pull Request Overview
This PR enables configuring a custom event store for React Native by allowing the injection of a custom async storage implementation instead of relying solely on AsyncStorage. The changes update the tests, types, tracker initialization, and plugins to support providing a custom async storage object.
- Updated tests to pass a custom asyncStorage
- Modified tracker and plugin code to use a configured asyncStorage
- Added a new AsyncStorage interface in the types
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| trackers/react-native-tracker/test/plugins/session.test.ts | Tests updated to provide asyncStorage when creating session plugins |
| trackers/react-native-tracker/test/plugins/app_install.test.ts | Added asyncStorage parameter to app install plugin tests |
| trackers/react-native-tracker/test/event_store.test.ts | Updated event store tests to support both default and custom asyncStorage cases |
| trackers/react-native-tracker/src/types.ts | Introduced AsyncStorage interface and an asyncStorage field in event store configuration |
| trackers/react-native-tracker/src/tracker.ts | Refactored tracker initialization to normalize configuration including asyncStorage |
| trackers/react-native-tracker/src/plugins/session/index.ts | Removed hardcoded AsyncStorage in favor of injected asyncStorage |
| trackers/react-native-tracker/src/plugins/app_install/index.ts | Updated app install plugin to use injected asyncStorage |
| trackers/react-native-tracker/src/event_store.ts | Refactored event store creation to use injected asyncStorage |
Files not reviewed (1)
- trackers/react-native-tracker/package.json: Language not supported
Comments suppressed due to low confidence (1)
trackers/react-native-tracker/src/tracker.ts:52
- The line 'AppLifecycleConfiguration;' appears to be extraneous and does not contribute to configuration normalization. It is recommended to remove this line to improve code clarity.
AppLifecycleConfiguration;
| @@ -43,6 +43,7 @@ | |||
| "test": "jest" | |||
| }, | |||
| "peerDependencies": { | |||
| "@react-native-async-storage/async-storage": "~2.0.0", | |||
There was a problem hiding this comment.
Okay, thank you for that reproduction!
This PR proposes an approach to avoid the intrinsic usage of AsyncStorage, as discussed in #1413. The why resides in the fact that the React Native community is now accustomed to using different solutions to keep data in local storage (e.g., https://github.com/mrousavy/react-native-mmkv, https://github.com/react-native-async-storage/async-storage).
Forcing the use of AsyncStorage might be undesirable, especially for projects bootstrapped with MMKV, as it would introduce a duplication of intents when dealing with multiple storage.