amplitude / Amplitude-JavaScript

JavaScript SDK for Amplitude
MIT License
315 stars 133 forks source link

Why React Native SDK is based on JS SDK? #236

Open alexmbp opened 4 years ago

alexmbp commented 4 years ago

Hello Amplitude!

What was the reason to implement react-native SDK on top of JS SDK? Probably it would be more correct to implement bridges around native SDK?

I worry that conditions might lead to possible issues in both browser/react-native. I should also mention that it's quite uncomfortable to exclude react-native dependencies from the package (not super bad, but not good).

Is it a temporary workaround or you are going to invest more in that solution?

Connected issues: https://github.com/amplitude/Amplitude-JavaScript/issues/207 - dependencies https://github.com/amplitude/Amplitude-JavaScript/issues/222 - features https://github.com/amplitude/Amplitude-JavaScript/issues/226 - features

haoliu-amp commented 4 years ago

Thanks for your feedback! We will discuss about this internally. Once we made a decision, we keep you updated!

If anyone else has the same request or concern, please leave comments or just hit a emoji reply.

nestorgm commented 4 years ago

I would like to know if there is actually a way to send events from a react-native application. The document in Amplitude "React Native SDK" is inaccurate and people from support has not been able to give me a precise way to use that SDK in my application. Did you @alexmbp were able to import amplitude-js and issue an amplitude.logEvent in a ReactNative application?

haoliu-amp commented 4 years ago

@nestorgm Sorry for the inconvenience! Are you talking about this one?

https://help.amplitude.com/hc/en-us/articles/360025220532-React-Native-SDK

nestorgm commented 4 years ago

Yes.

haoliu-amp commented 4 years ago

@nestorgm Where of this doc got you stuck? Could you give us more details?

nestorgm commented 4 years ago

Well. I'll try to explain myself. First I will say that, in the process, I created a workspace in the Amplitude service and have received events from an HTML API, from a web page, and from wrapped native Android and iOS. Those where made as a reference and to be sure that events were traveling from my site to Amplitude's workspace. Following that document, in my React Native application I added amplitude-js module. Then in the first screen imported it and issued the lines from example. That did'nt work. First think made me suspect something was wrong, was the line import amplitude 'amplitude-js' instead of import amplitude from 'amplitude-js' Then i try to initialize the connection, sending my device api key, but i have to guess with lines as amplitude.getInstance().init('26436d373f5efbc2abef4bf02e606a2b') and amplitude.init('26436d373f5efbc2abef4bf02e606a2b') because there were no more directions about how to do it.

In summary, I have made different intents in different ways but, none of they can send an event to reach Amplitude's workspace. All of the examples and references that support from Amplitude have given me are about web pages, pure JavaScript or native code. I have not seen a simple piece of code from a real React Amplitude application using this SDK.

alexmbp commented 4 years ago

@nestorgm make sure you have installed https://github.com/react-native-community/async-storage. It plays an important role in sending events. It seems for me you have missed that step.

npm install --save react-native-device-info npm install --save @react-native-community/async-storage

@haoliu-amp I agree with @nestorgm that docs does not include minimal required setup. I installed that packages because I was debugging your code.

alexmbp commented 4 years ago

I should also mention that code quality around that SDK is quite poor. It misses a lot of required checks/throws that should happen when set up is not correct.

Besides that I can say that code base does not follow the most important design principles. Your functions stopped to have single responsibility. Functions name don't represent what they supposed to do.

For instance: _saveCookieData

var _saveCookieData = function _saveCookieData(scope) {
  const cookieData = {
    deviceId: scope.options.deviceId,
    userId: scope.options.userId,
    optOut: scope.options.optOut,
    sessionId: scope._sessionId,
    lastEventTime: scope._lastEventTime,
    eventId: scope._eventId,
    identifyId: scope._identifyId,
    sequenceNumber: scope._sequenceNumber
  };
  if (AsyncStorage) {
    AsyncStorage.setItem(scope._storageSuffix, JSON.stringify(cookieData));
  }
  scope.cookieStorage.set(scope.options.cookieName + scope._storageSuffix, cookieData);
};

In some cases _saveCookieData does more than it's supposed to do. I mean it stores data in AsyncStorage.

It was probably right before adding ReactNative support (but I would argue even with that). I would say it should be something like:

var _saveScopeMeta = function _saveCookieData(scope) {
  const scopeMeta = {  ...  };
  this.storage.save(scopeMeta);
};

SDK should init 'right' storage based on platform, etc. It should make code more clean and readable.

alexmbp commented 4 years ago

React-Native support added lots of conditions in almost every SDKs method. I want to mention init method where we have nested conditions: if AsyncStorage -> if DeviceInfo. It seems to me those 2 are required for react-native, so if they are not available - init method should throw.

I think you should try to think about some kind of dependency injection for cases like that:

    let osName = this._ua.browser.name;
    let osVersion = this._ua.browser.major;
    let deviceModel = this._ua.device.model;
    let deviceManufacturer = this._ua.device.vendor;

    let versionName;
    let carrier;
    if (BUILD_COMPAT_REACT_NATIVE) {
      osName = Platform.OS;
      osVersion = Platform.Version;
      if (this.deviceInfo) {
        carrier = this.deviceInfo.carrier;
        deviceManufacturer = this.deviceInfo.manufacturer;
        deviceModel = this.deviceInfo.model;
        versionName = this.deviceInfo.version;
      }
    }

You can create some device info provider that follows some interface (I will write using TypeScript pseudocode because it's easier to read.):

interface DeviceInfo {
  osName: string;
  osVersion: string;
  deviceModel: string;
  deviceManufacturer: string;
}

interface DeviceInfoProvider {
  getOsName: string;
  getOsVersion: string;
  getDeviceModel: string;
  getDeviceManufacturer: string;
  getComposedInfo: DeviceInfo;
}

class UserAgentDeviceInfoProvider implements DeviceInfoProvider {
  private readonly _ua = new UAParser(navigator.userAgent).getResult();

  public getOsName() {
    return this._ua.browser.name;
  }
  public getComposedInfo() {
    return { osName: this.getOsName(), ... };
  }
}

class ReactNativeDeviceInfoProvider implements DeviceInfoProvider {
  private readonly _ua = new UAParser(navigator.userAgent).getResult();

  public getOsName() {
    return Platform.OS;
  }
  public getDeviceManufacturer() {
    return DeviceInfo.getManufacturer();
  }

  public getComposedInfo() {
    return {
      osName: this.getOsName(),
      deviceManufacturer: this.getDeviceManufacturer(),
      ...
    };
  }
}

This approach allows to build very flexible SDKs. It's easier to test, because you can create provider with mocked data. It's easier to debug, you can find where is an issue in a provider or in a method that uses it.

Same approach should be used for storage provider and any other entities that can be injected. It should also help to satisfy requests like that: https://github.com/amplitude/Amplitude-JavaScript/issues/221.

I believe that you have 2 possible ways of SDKs future development:

  1. Take a step back and think about architecture, create providers, inject more dependencies, throw if dependencies are not satisfied.
  2. Start brand new SDK wrappers around native SDKs.
haoliu-amp commented 4 years ago

@alexmbp Thanks for all your feedback, really helpful for us. We had a discussion over option 1 and 2 few weeks ago, and will start doing option 2 later this year.

alexmbp commented 4 years ago

@haoliu-amp Thanks for letting me know! I appreciate it. Just curios - what approach has been picked for the flutter SDK?

@nestorgm have you been able to send events using SDK?

nestorgm commented 4 years ago

Well, they send me an actual ReactNative example where I can test an prove that SDK is working and sending events. Unfortunately in my application, which is more complex, previous to RN 0.60 and not using new hooks approach, sending of events is not been accomplished. There is any errors and the instance is being updated, but nothing is sent through the network. Still trying to discover why. Thanks @alexmbp for your suggestion: -community/async-storage and -device-info are installed.

haoliu-amp commented 4 years ago

@alexmbp Unfortunately Flutter SDK was also not a wrapper solution, we might need to change it too.

haoliu-amp commented 4 years ago

@nestorgm Let us know if you need further help!

ghost commented 4 years ago

I'd also like to get the wrapper above the native code for React Native, not just JS SDK, because of the impossibility to add Amplitude.getInstance().trackSessionEvents(true); in JS SDK (in the opposite of Android/iOS SDKs).