Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React Native Expo Support #7

Closed
nicholascm opened this issue Aug 18, 2021 · 26 comments
Closed

React Native Expo Support #7

nicholascm opened this issue Aug 18, 2021 · 26 comments
Assignees
Labels
enhancement New feature or request released - NPM
Milestone

Comments

@nicholascm
Copy link

nicholascm commented Aug 18, 2021

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
Copy link
Contributor

MSNev commented Oct 12, 2021

@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
Copy link
Author

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

@MSNev
Copy link
Contributor

MSNev commented Dec 15, 2021

@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
Copy link
Author

@MSNev Sounds good - thanks for letting me know.

@DannyBiezen
Copy link

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

@nicholascm
Copy link
Author

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
Copy link
Contributor

MSNev commented Jul 25, 2022

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

@MSNev MSNev transferred this issue from microsoft/ApplicationInsights-JS Aug 2, 2022
@DannyBiezen
Copy link

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
Copy link
Author

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.

@MSNev
Copy link
Contributor

MSNev commented Aug 11, 2022

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
Copy link

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
Copy link
Contributor

MSNev commented Aug 12, 2022

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

@MSNev MSNev added enhancement New feature or request up-for-grabs This feature is not assigned to be implemented and is currently available. labels Aug 12, 2022
@nicholascm
Copy link
Author

I'm not sure that Metro supports tree shaking facebook/metro#227 (comment) - 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
Copy link
Contributor

l2aelba commented Aug 23, 2022

Any updates? thanks.

@alexanderdavide
Copy link

alexanderdavide commented Sep 8, 2022

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
Copy link

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

@luckywacky
Copy link

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:

  • we are using expo install rather than npm install to install the packages
  • appInsights.trackPageView() will log "undefined page" in Azure App Insights Panel - the SDK probably can't recognise routing in RN app out of the box.

@nicholascm
Copy link
Author

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
Copy link

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
Copy link
Contributor

MSNev commented Apr 12, 2023

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 MSNev removed the up-for-grabs This feature is not assigned to be implemented and is currently available. label May 22, 2023
@MSNev MSNev added this to the 4.0.0 milestone May 22, 2023
@MSNev
Copy link
Contributor

MSNev commented Jul 14, 2023

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.

siyuniu-ms added a commit that referenced this issue Jul 14, 2023
create seperate entry point for expo
@nicholascm
Copy link
Author

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!

@MSNev MSNev closed this as completed Jul 21, 2023
@julianurregoar
Copy link

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
Copy link
Contributor

MSNev commented Mar 27, 2024

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

@Karlie-777
Copy link
Contributor

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released - NPM
Projects
None yet
Development

No branches or pull requests

9 participants