Description
I opened #246 to address the fact that only a subset of the AsyncStorage is required for Mixpanel custom storage. However, I later found that, as long as useNative is true, the custom storage is irrelevant.
Unfortunately, there is a bug in the 3.0.2 code where, if @react-native-async-storage/async-storage isn't installed, a superfluous and incorrect error will always be logged:
[@RNC/AsyncStorage]: NativeModule: AsyncStorage is null. Please run 'npm install @react-native-async-storage/async-storage' or follow the Mixpanel guide to set up your own Storage class.
This error is logged because mixpanel-queue.js is an IIFE, that calls MixpanelPersistent.getInstance() without a storage argument. Meaning, it assumes that you'll have the async-storage package installed.
This all runs because it's imported, ultimately, by mixpanel-main.js, which is imported by index.js. So, even if MixpanelMain is never used, because useNative is true, it will still initialize this superfluous queue and log the error.
That's the core of the issue, which is broken for anyone who doesn't have @react-native-async-storage/async-storage installed, whether they use native, or custom storage, etc.
To me, there are sort of larger issues as well. From an API standpoint, it's not immediately clear that useNative and storage are related, or in other words that: If I set useNative to false, passing storage will allow me to use a custom "non-native" storage implementation. To boil this point down into types, the constructor should be something like this:
class Mixpanel {
constructor(token: string, trackAutoMaticEvents: boolean)
constructor(token: string, trackAutoMaticEvents: boolean, useNative: true)
constructor(
token: string,
trackAutoMaticEvents: boolean,
useNative: false,
nonNativeStorage?: MixpanelAsyncStorage,
)
constructor(
token: string,
trackAutomaticEvents: boolean,
useNative?: boolean,
nonNativeStorage?: MixpanelAsyncStorage,
) { /* implementation */ }
}
// Usage
const mpNative1 = new Mixpanel(token, true) // ok
const mpNative2 = new Mixpanel(token, true, true) // ok
const mpNative3 = new Mixpanel(token, true, true, customStorage) // error
const mpNative4 = new Mixpanel(token, true, undefined, customStorage) // error
const mpNative5 = new Mixpanel(token, true, false, customStorage) // okFrom a types perspective at least, this would clear up confusion with how useNative and storage are related. This could also be clarified a bit more in the docs.
Right now, however, the more pressing concern is the IIFE error.