Append this._storageSuffix (which includes API key) to unsentKey #204
Conversation
…re storing in AsyncStorage Currently, when saving events to AsyncStorage, we we are only using unsentKey as the identifier. This can cause events from multiple projects to get mixed up. By appending this._storageSuffix, we are adding more specificity to keep unsent events within the correct project scope.
…add suffix to current events keyname
| } | ||
| }).catch((err) => { | ||
| this.options.onError(err); | ||
| }); |
There was a problem hiding this comment.
this is all wrapped in the callback of the _migrateUnsentEvents
There was a problem hiding this comment.
Can you add a few tests around the migration? I want to be 100% sure that we don't lose or corrupt the unsent event / identify data.
Some potential test cases:
- normal migration of event / identify in old key to new key, and validate old key gone, new key has stuff
- migration is skipped if stuff is already migrated / or nothing to migrate
- also make sure this doesn't mess up the people using localstorage / not using Async?
Also recommend @blazzy taking a look
| try { | ||
| if (AsyncStorage) { | ||
| AsyncStorage.setItem(this.options.unsentKey, JSON.stringify(this._unsentEvents)); | ||
| AsyncStorage.setItem(this.options.unsentKey + this._storageSuffix, JSON.stringify(this._unsentEvents)); |
There was a problem hiding this comment.
not blocking but in terms of API I feel like _setInStorage should encapsulate the AsyncStorage logic inside it, so that whoever calls _setInStorage doesn't have to worry about checking Async etc etc you know what I mean?
| /** | ||
| * @private | ||
| */ | ||
| AmplitudeClient.prototype._migrateUnsentEvents = function _migrateUnsentEvents(cb) { |
There was a problem hiding this comment.
I'm curious why we have this callback chaining pattern in init? Is it because we need it to interact with AsyncStorage? in the case where init is just a consecutive / synchronous you could just write the logic all straight out without any callbacks.
There was a problem hiding this comment.
to make sure they're not touching asyncStorage at the same time, e.g. making it synchronous via migrating unsent events before accessing it
| Promise.all([ | ||
| AsyncStorage.getItem(this.options.unsentKey), | ||
| AsyncStorage.getItem(this.options.unsentIdentifyKey), | ||
| ]).then((values) => { |
There was a problem hiding this comment.
Are you running this migration every single time the SDK inits? You only need to run it one time, which is when the user loads this new SDK for the first time. Do you need to check if values is null or something for all the subsequent times it's run?? You could also just check that the unsentKey and unsentIdentifyKey exist in Async storage, if not just skip this entire code block?
src/amplitude-client.js
Outdated
| var unsentIdentifyKey = values[1]; | ||
| Promise.all([ | ||
| AsyncStorage.setItem(this.options.unsentKey + this._storageSuffix, unsentEventsString), | ||
| AsyncStorage.setItem(this.options.unsentIdentifyKey + this._storageSuffix, unsentIdentifyKey ), |
There was a problem hiding this comment.
extra space after unsentIdentifyKey
side note: would be useful to add a linter or prettier to the JS SDK?
|
@djih updated with the migration. i think it's going to be a lot of effort to add the react-native specific tests + configs. I want to prioritize getting it out for now and I'll do a follow up on testing |
sputh
force-pushed
the
better-react-native-async-storage
branch
from
1d2bd6c to
fe025f0
Compare
2 participants
Currently, when saving events to AsyncStorage, we we are only using unsentKey as the identifier.
This can cause events from multiple projects to get mixed up. By appending this._storageSuffix,
we are adding more specificity to keep unsent events within the correct project scope.
Addresses: #203
FYI: no need to do this for web because it is calling
_setInStorage()which utilizesthis._storageSuffixalready