microsoft / applicationinsights-react-native

Microsoft Application Insights React Native Plugin
MIT License
14 stars 5 forks source link

React Native Expo Support #7

Closed nicholascm closed 11 months ago

nicholascm commented 2 years ago

Is your feature request related to a problem? Please describe.

Looking through the code for the react-native plugin, I didn't find much that would preclude support for Expo. The main issue I saw is the peer dependency on react-native-device-info, but if the plugin allowed the consumer to inject something which meets that same interface at run time(e.g. something like IDeviceInfo), I couldn't see a reason that expo would not work. Expo exposes all the data needed to satisfy that interface using libraries which are Expo-compatible.

Is there something I have missed?

I'd be excited to open a PR or create a separate extension and share code between the two if a separate extension or alternative approach was preferred to avoid a breaking change.

Describe the solution you'd like

Consumers of this library for RN can inject their own service to provide the device info at initialization, removing need for peer dependency on react-native-device-info. This would be a breaking change for current consumers.

Describe alternatives you've considered

I'd like to use this package for expo and it is explicitly noted that expo will not work. The other option would be to create a separate plugin, however, the two would be nearly identical with the exception of how device info is initialized.

Additional context

MSNev commented 2 years ago

@xiao-lix can you please review this and suggest whether we can abstract out the device info collection as suggested or whether a separate plugin would be the better approach.

A separate extension could (possibly) be built in the same folder (abstracting out a common base class - perhaps) and generating and exporting 2 different plugins as we only publish this via NPM and consuming environment should then tree-shake out the unused instance.

nicholascm commented 2 years ago

Hi @MSNev / @xiao-lix - any update/thoughts on this?

MSNev commented 2 years ago

@nicholascm to give you some context, internally we are a relatively small team and have limited bandwidth to support a complete set of SDK's and therefore we have to either provide some generic guidance / solution and rely on the community to help guide us of which frameworks to support.

In addition to this @xiao-lix has now left the team and therefore we have to backfill their position, so it is currently unlikely that we will be able to code anything ourselves in the short term.

Based on your description, please feel free to submit a PR for the sort of changes you are thinking of đŸ˜„

nicholascm commented 2 years ago

@MSNev Sounds good - thanks for letting me know.

DannyBiezen commented 1 year ago

Are there any plans of supporting this in the near future? It would be great to have AppInsights support in Expo!

nicholascm commented 1 year ago

I have a fork of this that I should probably open a PR for - its pretty easy to remove the offending dependency here, it just happens to be a breaking change.

MSNev commented 1 year ago

Nice! Note we are in the process of moving react-native into it's own repo (https://github.com/microsoft/applicationinsights-react-native), I've already moved all of the history over (using git to merge the unrelated history), so further changes to the react-native extensions won't occur (in this repo).

We got the final legal approval sign-off this morning, so I just need some else from the team to approve the open PR which does the merge and after that it will be (almost) live -- targeting the end of this week to be able to release an unpinned NPM package.

So please wait before opening a PR to add Expo support. We will also be bumping the version of react-native used (after the unpinning release).

DannyBiezen commented 1 year ago

I noticed that version 3.0.0 was released which added setDeviceInfoModule which is great! I've tried to use this in my Expo app using the following code

const myDeviceInfoModule: IDeviceInfoModule = {
  getUniqueId: () => "id",
  getModel: () => "model",
  getDeviceType: () => "deviceType",
};
const RNPlugin = new ReactNativePlugin();
RNPlugin.setDeviceInfoModule(myDeviceInfoModule);
const appInsights = new ApplicationInsights({
  config: {
    connectionString: "...",
    extensions: [RNPlugin],
  },
});
appInsights.loadAppInsights();

When running this I get the following error: image

I also tried installing react-native-device-info just to see if that would work, but doing that gives me a different error: image

Any ideas on how to fix this? I can create a separate plugin for this which removes all references react-native-device-info but would prefer to keep using this package.

nicholascm commented 1 year ago

Yeah - I think since this package still falls back on / has import references to react-native-device-info, metro has issues bundling when it is not present.

https://github.com/microsoft/applicationinsights-react-native/blob/a89453037e0949b4def3aec1696a361d710aed25/applicationinsights-react-native/src/DeviceInfo/ReactNativeDeviceInfo.ts#L4

MSNev commented 1 year ago

One of the reasons for the new interface was to enable testing of this in the browser and I hit a similar issue (because the browser isn't React-Native) and as a work around the tests to 2 things 1) They explicitly set the "imported" module as the web version https://github.com/microsoft/applicationinsights-react-native/blob/main/applicationinsights-react-native/Tests/UnitTests.html#L47 2) Explicitly use the "DeviceModule" https://github.com/microsoft/applicationinsights-react-native/blob/main/applicationinsights-react-native/src/DeviceInfo/DeviceModule.ts and set it (like you have above) https://github.com/microsoft/applicationinsights-react-native/blob/main/applicationinsights-react-native/Tests/Unit/src/reactnativeplugin.tests.ts#L21-L22

This causes the ReactNativeDeviceInfo to never get used or referenced.

Based on the error your getting it seems like there is some sort of static initialization that is getting run..

The code for the ReactNativeDeviceInfo is probably still getting included (and not tree-shaken away) because of the default "fallback" usage of getReactNativeDeviceInfo() (for backward compatibility) https://github.com/microsoft/applicationinsights-react-native/blob/main/applicationinsights-react-native/src/DeviceInfo/ReactNativeDeviceInfo.ts

So I think the additional "trick" is going to revolve around causing ReactNativeDeviceInfo to not run whatever is causing this static initialization.

Depending on your packaging "if" it is using require() to load "react-native-device-info" then you could "hack" (which is effectively what the tests are doing) to define an "empty" DeviceInfo default object (as it will never get called once you do this RNPlugin.setDeviceInfoModule(myDeviceInfoModule);

DannyBiezen commented 1 year ago

I ended up using patch package to remove the dependency on react-native-device-info which is not ideal but it resolves the issue and I am now able to use this package in my Expo project!

MSNev commented 1 year ago

Right, not ideal...

I guess one possible approach now that we have the abstraction interface might be to shift the bulk of the plugin to a new class name and create a new sub-class using the existing "name" (so we don't break backward compatibility) which if not set populates the default using the react-native-device-info.

In theory then if you only used the "base" class the other one "should" cause all of the react-native-device-info references to get tree-shaken and dropped...

I think the only change in the existing (to become "base" class) would be to provide a hook for this line https://github.com/microsoft/applicationinsights-react-native/blob/main/applicationinsights-react-native/src/ReactNativePlugin.ts#L113 that sub-classes could implement / override...

nicholascm commented 1 year ago

I'm not sure that Metro supports tree shaking https://github.com/facebook/metro/issues/227#issuecomment-583358386 - I haven't found anything more current that indicates that this has changed, so that may be an issue. I'll try and find some time this week to try out different configurations in our project with v3.0 and see what happens.

l2aelba commented 1 year ago

Any updates? thanks.

alexanderdavide commented 1 year ago

Side note regarding Expo compatibility: I find the documentation "This plugin will only work in react-native apps, e.g. it will not work with expo." misleading because Expo bare workflow works perfectly fine. Of course, Expo bare workflow is just React Native with Expo SDK support but the documentation might discourage people who are not entirely familiar with Expo's workflows.

luckywacky commented 1 year ago

@nicholascm @MSNev did you have any luck configuring your expo projects to work with this library (without bare workflow)?

luckywacky commented 1 year ago

We have successfully configured our Microsoft Insights with Expo (development build) without the need to remove react-native-device-info dependency.

Steps:

  1. install @microsoft SDK's: expo install @microsoft/applicationinsights-react-native @microsoft/applicationinsights-web
  2. install the problematic package: expo install react-native-device-info
  3. create a custom build eas build --{your profile} --platform {your platform}

we do not need to use react-native link react-native-device-info - expo's Autolinking does the job for us.

we made a small singleton service to cover it all:

import { ReactNativePlugin } from "@microsoft/applicationinsights-react-native";
import { ApplicationInsights } from "@microsoft/applicationinsights-web";

const connectionString = "your connection string";

class InsightsService {
  private appInsights: ApplicationInsights;
  constructor() {
    const RNPlugin = new ReactNativePlugin();
    this.appInsights = new ApplicationInsights({
      config: {
        connectionString,
        extensions: [RNPlugin],
      },
    });
    this.appInsights.loadAppInsights();
  }

  logEvent(name: string, properties?: { [k: string]: unknown }) {
    this.appInsights.trackEvent({ name, properties });
  }
  logException(exception: Error, severityLevel?: number) {
    this.appInsights.trackException({ exception, severityLevel });
  }
}

export const insightService = new InsightsService();

important notes:

nicholascm commented 1 year ago

This seems like it should only work if you're already using a custom development client - is that the case here?

For folks who are managed workflow with Expo Go, I don't think react-native-device-info installation is an option. I modified this package locally for our project to delete react-native-device-info and swap in expo-device and it is pretty simple to do, but it constitutes a breaking change for this library.

Also in case you were looking - you can use something like this: https://reactnavigation.org/docs/screen-tracking/ if you're using react-navigation.

luckywacky commented 1 year ago

This seems like it should only work if you're already using a custom development client - is that the case here?

Yes. That is the case.

MSNev commented 1 year ago

This also sounds like a breaking change to NOT automatically expect the Device Info module to be present and rather always require that you inject it...

Sound like a v4.x will be required...

MSNev commented 11 months ago

Hi all, we have a PR available which should address this issue #25, if you get a chance can you please provide a 2nd set of eyes on what we are proposing.

nicholascm commented 11 months ago

It looks good to me - seems like anyone (even outside of expo users) can use the "manual" plugin if they just want to have control over how they get their device info. Thanks @MSNev!

julianurregoar commented 3 months ago

Hi @MSNev !

Has this issue been completely resolved?

I found following message in the package, so seems like it can not be used on Expo.

You must be using a version >=2.0.0 of @microsoft/applicationinsights-web. This plugin will only work in react-native apps, e.g. it will not work with expo.

Is that correct?

Thanks!

MSNev commented 3 months ago

@siyuniu-ms, @Karlie-777 can you please try and answer the above question?

Karlie-777 commented 3 months ago

@julianurregoar Which version of app Insights is used for your app? React Native Expo support was added after version 4.0.0 which requires appInsights 3.x.x.

julianurregoar commented 3 months ago

@Karlie-777 Thanks! I will proceed with the installation.