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

Created analytics module #6663

Closed
wants to merge 46 commits into from
Closed

Conversation

@EvanBacon
Copy link
Contributor

EvanBacon commented Jan 3, 2020

Why

Utilize some of the existing Google Services that we include with expo-notifications.

How

  • Created expo-firebase-analytics (from scratch) which does the most bare-bones binding to the native Firebase analytics packages.
  • Included a small method for initializing an app in the managed workflow on iOS.

Test Plan

  • [bare-workflow][ios, android, web] Running the native app with bare-expo should show analytics working in the Firebase console. You'll want to start the native app in debug mode (this should modify apps/bare-expo/ios/BareExpo.xcodeproj/xcshareddata/xcschemes/BareExpo.xcscheme).
  • [managed-workflow][ios] Start by deleting or using a different GoogleServices-info.plist in bare-expo then build the app and use the following to start the native iOS app at runtime (this seems to not work for analytics on Android) (bare expo currently runs this in CI on iOS)
if (Platform.OS === 'ios' && global.__DEV__ === true) {
  await Analytics.initializeAppDangerously({ /* GoogleServices-info.plist contents as JSON. */ });
}
@EvanBacon EvanBacon self-assigned this Jan 3, 2020
@EvanBacon EvanBacon requested review from ide, sjchmiela and brentvatne Jan 3, 2020
EvanBacon added 8 commits Jan 3, 2020
@EvanBacon EvanBacon marked this pull request as ready for review Jan 4, 2020
@EvanBacon EvanBacon requested a review from tsapeta as a code owner Jan 4, 2020
@EvanBacon EvanBacon requested a review from IjzerenHein Jan 8, 2020
Copy link
Contributor

sjchmiela left a comment

That is a solid pull request! I left some comments, I hope you find them useful. 🙂

Copy link
Contributor

sjchmiela left a comment

That is a solid pull request! I left some comments, I hope you find them useful. 🙂

Copy link
Contributor

IjzerenHein left a comment

I'm still wrapping my mind around everything that is Expo, but here are couple comments so far from my side.

Generally, everything looks pretty good. 👍 As for the Firebase App initialization, we should probably move that to a separate module later on, but it's find to keep this in here for now.

@@ -55,6 +55,8 @@ export function getTestModules() {

if (Platform.OS === 'android') {
modules.push(require('./tests/JSC'));
} else if (Platform.OS === 'ios') {
modules.push(require('./tests/FirebaseAnalytics'));

This comment has been minimized.

Copy link
@IjzerenHein

IjzerenHein Jan 10, 2020

Contributor

else should indeed be removed here, implies that there is a relationship with the JSC test above, but there isn't.

Currently all the tests fail on Android, so it's better to keep them disabled until fixed.

packages/expo-firebase-analytics/src/Analytics.ts Outdated Show resolved Hide resolved
@cemo

This comment has been minimized.

Copy link

cemo commented Jan 13, 2020

@EvanBacon we are interested in all Firebase services in managed flow. Would you please explain what will change after this PR will be merged for us? :)

@IjzerenHein

This comment has been minimized.

Copy link
Contributor

IjzerenHein commented Feb 26, 2020

Moved to #7017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.