Console errors: 3.0.2 support of custom storage is broken and confusing

[bfricka]

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) // ok

From 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.

[bfricka]

Note, that, not wanting to refactor a bunch of stuff myself, I patched the types, and removed this issue the "easy" way, by using a dynamic require, and removing the import:

diff --git a/index.d.ts b/index.d.ts
index 20f63da6353d893cf38a93e5e366e24f7ab9da9f..7bee0c832bd51f812f98d7c637825af5aedfffaf 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -1,12 +1,20 @@
 type MixpanelType = any;
 type MixpanelProperties = {[key: string]: MixpanelType};

+export type MixpanelAsyncStorage = {
+  getItem(key: string): Promise<string | null>
+  setItem(key: string, value: string): Promise<void>
+  removeItem(key: string): Promise<void>
+}
+
 export class Mixpanel {
+  constructor(token: string, trackAutoMaticEvents: boolean)
+  constructor(token: string, trackAutoMaticEvents: boolean, useNative: true)
   constructor(
     token: string,
-    trackAutomaticEvents: boolean,
-    useNative?: boolean,
-    storage?: any
+    trackAutoMaticEvents: boolean,
+    useNative: false,
+    nonNativeStorage?: MixpanelAsyncStorage,
   );
   static init(
     token: string,
diff --git a/index.js b/index.js
index 4fc0dd6f01ae31fd9487f012c56bec2f079b7405..279aae2f6190dae4aaf54f8408024926d7a1a230 100644
--- a/index.js
+++ b/index.js
@@ -3,7 +3,6 @@
 import {Platform, NativeModules} from "react-native";
 import packageJson from "./package.json";
 const {MixpanelReactNative} = NativeModules;
-import MixpanelMain from "mixpanel-react-native/javascript/mixpanel-main";

 const DevicePlatform = {
   Unknown: "Unknown",
@@ -46,27 +45,25 @@ export class Mixpanel {
     }
     this.token = token;
     this.trackAutomaticEvents = trackAutomaticEvents;
-    if (!useNative) {
-      this.mixpanelImpl = new MixpanelMain(
-        token,
-        trackAutomaticEvents,
-        storage
-      );
+
+    if (useNative && MixpanelReactNative) {
+      this.mixpanelImpl = MixpanelReactNative;
       return;
     }

-    if (!MixpanelReactNative) {
+    // useNative is true, but MixpanelReactNative doesn't exist
+    if (useNative) {
       console.warn(
         "MixpanelReactNative is not available; using JavaScript mode. If you prefer not to use the JavaScript mode, please follow the guide in the GitHub repository: https://github.com/mixpanel/mixpanel-react-native."
       );
-      this.mixpanelImpl = new MixpanelMain(
-        token,
-        trackAutomaticEvents,
-        storage
-      );
-    } else {
-      this.mixpanelImpl = MixpanelReactNative;
     }
+
+    const MixpanelMain = require('mixpanel-react-native/javascript/mixpanel-main');
+    this.mixpanelImpl = new MixpanelMain(
+      token,
+      trackAutomaticEvents,
+      storage
+    );
   }

   /**

[christophemenager]

@zihejia this issue is not solved, v3 is still broken for users who use useNative=true without AsyncStorage

[bfricka]

@zihejia this issue is not solved, v3 is still broken for users who use useNative=true without AsyncStorage

Agreed. This isn't going to be fixed until they remove the IIFE from mixpanel-queue. They should make that a singleton if they want to use it like one. That way instantiation can be deferred, and they can also pass the appropriate options to it.