-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Adding AngularFireAnalytics, AngularFireRemoteConfig, and refactoring DI Tokens #2187
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
Merged
Merged
Changes from 25 commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
e5be927
Firebase v7, analytics and remote-config
jamesdaniels 1cf961b
Cleaning up the DI tokens
jamesdaniels f5f67ad
Cleaning things up
jamesdaniels 73413e8
DI and jazz
jamesdaniels dd0efb1
Bumping the tests
jamesdaniels dffca53
Adding to the root-spec
jamesdaniels 9731e3c
Spelling is good.
jamesdaniels 7308f9c
Merge branch 'master' into firebase-v7
jamesdaniels dcdf8bc
Have to include UMDs in the karma.conf
jamesdaniels 6f71d46
Just importing performance is destablizing the app
jamesdaniels 6638b9d
Merge branch 'master' into firebase-v7
jamesdaniels d7d52c8
Adding the zone arg to the app factory
jamesdaniels eb4bc00
First pass on the new RC API and the start of the AngularFireLazy effort
jamesdaniels 89344cc
Update src/remote-config/tsconfig-test.json
jamesdaniels 075afe6
Reworking things a bit
jamesdaniels b8b351a
Router as optional, drop this/private from screen tracking
jamesdaniels 1e39052
Minor changes to RC
jamesdaniels 768c21b
It's firebase_screen_class
jamesdaniels 60c0cad
Reworking analytics
jamesdaniels 9b2e920
current!
jamesdaniels 916e069
Use _loadedConfig if available and scope screen_id on outlet
jamesdaniels 5955925
Fixing the types to handle older Firebase SDKs
jamesdaniels 659165e
SEMVER notes on the DI tokens
jamesdaniels 62d90b9
Starting on the docs
jamesdaniels 1a43ad1
Merge branch 'firebase-v7' of github.com:angular/angularfire2 into fi…
jamesdaniels 67e1b55
Monitoring...
jamesdaniels 91778ff
More work on analytics
jamesdaniels 460170c
more expirimentation
jamesdaniels 3c1ad1f
Flushing out RC more and fixing SSR issues
jamesdaniels e2d83c8
New RC API
jamesdaniels 4601932
Mapping to objects and templates, budget pipe
jamesdaniels 7fe92ed
More strict types
jamesdaniels d47dc3f
Fix proxy in Node, get component name for analytics in both JIT and AOT
jamesdaniels 05c840b
Cleaning things up, beyond docs ready for release
jamesdaniels b46b382
Docs and cleanup
jamesdaniels ff65db8
Fixing analytics spec
jamesdaniels baaeccf
Adding more API to the docs
jamesdaniels 04a3bb1
Further simplifications to the DI tokens
jamesdaniels c37fcb7
Add Analytics and RemoteConfig to install-and-setup
jamesdaniels 6753a67
Merge branch 'master' into firebase-v7
jamesdaniels 6f114c6
Our RC Value implements a Partial, so minors dont break
jamesdaniels File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Getting started with Google Analytics | ||
|
||
### Something, something | ||
|
||
TBD | ||
|
||
### Putting it all together | ||
|
||
```ts | ||
@NgModule({ | ||
imports: [ | ||
AngularFireModule.initializeApp(environment.firebase), | ||
AngularFireAnalyticsModule | ||
], | ||
providers: [ | ||
ScreenTrackingService, | ||
UserTrackingService | ||
] | ||
}) | ||
export class AppModule { } | ||
``` | ||
|
||
```ts | ||
constructor(analytics: AngularFireAnalytics) { | ||
analytics.logEvent('custom_event', { ... }); | ||
} | ||
``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# Getting started with Remote Config | ||
|
||
### Something, something | ||
|
||
TBD | ||
|
||
### Putting it all together | ||
|
||
```ts | ||
@NgModule({ | ||
imports: [ | ||
AngularFireModule.initializeApp(environment.firebase), | ||
AngularFireRemoteConfigModule | ||
], | ||
providers: [ | ||
{ provide: DEFAULT_CONFIG, useValue: { enableAwesome: true } }, | ||
{ | ||
provide: REMOTE_CONFIG_SETTINGS, | ||
useFactory: () => isDevMode ? { minimumFetchIntervalMillis: 1 } : {} | ||
} | ||
] | ||
}) | ||
export class AppModule { } | ||
``` | ||
|
||
```ts | ||
constructor(remoteConfig: AngularFireRemoteConfig) { | ||
remoteConfig.changes.subscribe(changes => …); | ||
} | ||
``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { NgModule, Optional } from '@angular/core'; | ||
import { UserTrackingService, ScreenTrackingService } from './analytics.service'; | ||
import { AngularFireAnalytics } from './analytics'; | ||
|
||
@NgModule() | ||
export class AngularFireAnalyticsModule { | ||
constructor( | ||
analytics: AngularFireAnalytics, | ||
@Optional() screenTracking: ScreenTrackingService, | ||
@Optional() userTracking: UserTrackingService | ||
) { } | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
import { Injectable, Inject, Optional, NgZone, OnDestroy, InjectionToken } from '@angular/core'; | ||
import { Subscription, from, Observable, empty, of } from 'rxjs'; | ||
import { filter, withLatestFrom, switchMap, map, tap, pairwise, startWith, groupBy, mergeMap } from 'rxjs/operators'; | ||
import { Router, NavigationEnd, ActivationEnd } from '@angular/router'; | ||
import { runOutsideAngular, _lazySDKProxy, _firebaseAppFactory } from '@angular/fire'; | ||
import { AngularFireAnalytics } from './analytics'; | ||
import { User } from 'firebase/app'; | ||
|
||
export const APP_VERSION = new InjectionToken<string>('angularfire2.analytics.appVersion'); | ||
export const APP_NAME = new InjectionToken<string>('angularfire2.analytics.appName'); | ||
|
||
const DEFAULT_APP_VERSION = '?'; | ||
const DEFAULT_APP_NAME = 'Angular App'; | ||
|
||
type AngularFireAnalyticsEventParams = { | ||
app_name: string; | ||
firebase_screen_class: string | undefined; | ||
firebase_screen: string; | ||
app_version: string; | ||
screen_name: string; | ||
outlet: string; | ||
url: string; | ||
}; | ||
|
||
@Injectable({ | ||
providedIn: 'root' | ||
}) | ||
export class ScreenTrackingService implements OnDestroy { | ||
|
||
private disposable: Subscription|undefined; | ||
|
||
constructor( | ||
analytics: AngularFireAnalytics, | ||
@Optional() router:Router, | ||
@Optional() @Inject(APP_VERSION) providedAppVersion:string|null, | ||
@Optional() @Inject(APP_NAME) providedAppName:string|null, | ||
zone: NgZone | ||
) { | ||
if (!router) { return this } | ||
const app_name = providedAppName || DEFAULT_APP_NAME; | ||
const app_version = providedAppVersion || DEFAULT_APP_VERSION; | ||
const activationEndEvents = router.events.pipe(filter<ActivationEnd>(e => e instanceof ActivationEnd)); | ||
const navigationEndEvents = router.events.pipe(filter<NavigationEnd>(e => e instanceof NavigationEnd)); | ||
this.disposable = navigationEndEvents.pipe( | ||
withLatestFrom(activationEndEvents), | ||
switchMap(([navigationEnd, activationEnd]) => { | ||
const url = navigationEnd.url; | ||
const screen_name = activationEnd.snapshot.routeConfig && activationEnd.snapshot.routeConfig.path || url; | ||
const params: AngularFireAnalyticsEventParams = { | ||
app_name, app_version, screen_name, url, | ||
firebase_screen_class: undefined, | ||
firebase_screen: screen_name, | ||
outlet: activationEnd.snapshot.outlet | ||
}; | ||
const component = activationEnd.snapshot.component; | ||
const routeConfig = activationEnd.snapshot.routeConfig; | ||
const loadedConfig = routeConfig && (routeConfig as any)._loadedConfig; | ||
const loadChildren = routeConfig && routeConfig.loadChildren; | ||
if (component) { | ||
return of({...params, firebase_screen_class: nameOrToString(component) }); | ||
} else if (loadedConfig && loadedConfig.module && loadedConfig.module._moduleType) { | ||
return of({...params, firebase_screen_class: nameOrToString(loadedConfig.module._moduleType)}); | ||
} else if (typeof loadChildren === "string") { | ||
// TODO is this an older lazy loading style parse | ||
return of({...params, firebase_screen_class: loadChildren }); | ||
} else if (loadChildren) { | ||
// TODO look into the other return types here | ||
return from(loadChildren() as Promise<any>).pipe(map(child => ({...params, firebase_screen_class: nameOrToString(child) }))); | ||
} else { | ||
// TODO figure out what forms of router events I might be missing | ||
return of(params); | ||
} | ||
}), | ||
tap(params => { | ||
// TODO perhaps I can be smarter about this, bubble events up to the nearest outlet? | ||
if (params.outlet == "primary") { | ||
// TODO do I need to add gtag config for firebase_screen, firebase_screen_class, firebase_screen_id? | ||
// also shouldn't these be computed in the setCurrentScreen function? prior too? | ||
// do we want to be logging screen name or class? | ||
analytics.setCurrentScreen(params.screen_name, { global: true }) | ||
} | ||
}), | ||
map(params => ({ firebase_screen_id: nextScreenId(params), ...params})), | ||
groupBy(params => params.outlet), | ||
mergeMap(group => group.pipe(startWith(undefined), pairwise())), | ||
map(([prior, current]) => prior ? { | ||
firebase_previous_class: prior.firebase_screen_class, | ||
firebase_previous_screen: prior.firebase_screen, | ||
firebase_previous_id: prior.firebase_screen_id, | ||
...current! | ||
} : current!), | ||
tap(params => analytics.logEvent('screen_view', params)), | ||
runOutsideAngular(zone) | ||
).subscribe(); | ||
} | ||
|
||
ngOnDestroy() { | ||
if (this.disposable) { this.disposable.unsubscribe(); } | ||
} | ||
|
||
} | ||
|
||
@Injectable({ | ||
providedIn: 'root' | ||
}) | ||
export class UserTrackingService implements OnDestroy { | ||
|
||
private disposable: Subscription|undefined; | ||
|
||
// TODO a user properties injector | ||
constructor( | ||
analytics: AngularFireAnalytics, | ||
zone: NgZone | ||
) { | ||
this.disposable = from(analytics.app).pipe( | ||
// TODO can I hook into auth being loaded... | ||
map(app => app.auth()), | ||
switchMap(auth => auth ? new Observable<User>(auth.onAuthStateChanged.bind(auth)) : empty()), | ||
switchMap(user => analytics.setUserId(user ? user.uid : null!, { global: true })), | ||
runOutsideAngular(zone) | ||
).subscribe(); | ||
} | ||
|
||
ngOnDestroy() { | ||
if (this.disposable) { this.disposable.unsubscribe(); } | ||
} | ||
} | ||
|
||
// firebase_screen_id is an INT64 but use INT32 cause javascript | ||
const randomInt32 = () => Math.floor(Math.random() * (2**32 - 1)) - 2**31; | ||
|
||
const currentScreenIds: {[key:string]: number} = {}; | ||
|
||
const nextScreenId = (params:AngularFireAnalyticsEventParams) => { | ||
const scope = params.outlet; | ||
if (currentScreenIds.hasOwnProperty(scope)) { | ||
return ++currentScreenIds[scope]; | ||
} else { | ||
const ret = randomInt32(); | ||
currentScreenIds[scope] = ret; | ||
return ret; | ||
} | ||
} | ||
|
||
const nameOrToString = (it:any): string => it.name || it.toString(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import { ReflectiveInjector, Provider } from '@angular/core'; | ||
import { TestBed, inject } from '@angular/core/testing'; | ||
import { FirebaseApp, FirebaseOptionsToken, AngularFireModule, FirebaseNameOrConfigToken } from '@angular/fire'; | ||
import { AngularFireAnalytics, AngularFireAnalyticsModule, AUTOMATICALLY_SET_CURRENT_SCREEN, AUTOMATICALLY_LOG_SCREEN_VIEWS, ANALYTICS_COLLECTION_ENABLED, AUTOMATICALLY_TRACK_USER_IDENTIFIER, APP_VERSION, APP_NAME } from '@angular/fire/analytics'; | ||
import { COMMON_CONFIG } from './test-config'; | ||
|
||
|
||
describe('AngularFireAnalytics', () => { | ||
let app: FirebaseApp; | ||
let analytics: AngularFireAnalytics; | ||
|
||
beforeEach(() => { | ||
TestBed.configureTestingModule({ | ||
imports: [ | ||
AngularFireModule.initializeApp(COMMON_CONFIG), | ||
AngularFireAnalyticsModule | ||
] | ||
}); | ||
inject([FirebaseApp, AngularFireAnalytics], (app_: FirebaseApp, _analytics: AngularFireAnalytics) => { | ||
app = app_; | ||
analytics = _analytics; | ||
})(); | ||
}); | ||
|
||
afterEach(done => { | ||
app.delete(); | ||
done(); | ||
}); | ||
|
||
it('should be exist', () => { | ||
expect(analytics instanceof AngularFireAnalytics).toBe(true); | ||
}); | ||
|
||
it('should have the Firebase Functions instance', () => { | ||
expect(analytics.analytics).toBeDefined(); | ||
}); | ||
|
||
}); | ||
|
||
const FIREBASE_APP_NAME_TOO = (Math.random() + 1).toString(36).substring(7); | ||
|
||
describe('AngularFireAnalytics with different app', () => { | ||
let app: FirebaseApp; | ||
let analytics: AngularFireAnalytics; | ||
|
||
beforeEach(() => { | ||
TestBed.configureTestingModule({ | ||
imports: [ | ||
AngularFireModule.initializeApp(COMMON_CONFIG), | ||
AngularFireAnalyticsModule | ||
], | ||
providers: [ | ||
{ provide: FirebaseNameOrConfigToken, useValue: FIREBASE_APP_NAME_TOO }, | ||
{ provide: FirebaseOptionsToken, useValue: COMMON_CONFIG }, | ||
{ provide: AUTOMATICALLY_SET_CURRENT_SCREEN, useValue: true } | ||
{ provide: AUTOMATICALLY_LOG_SCREEN_VIEWS, useValue: true }, | ||
{ provide: ANALYTICS_COLLECTION_ENABLED, useValue: true }, | ||
{ provide: AUTOMATICALLY_TRACK_USER_IDENTIFIER, useValue: true }, | ||
{ provide: APP_VERSION, useValue: '0.0' }, | ||
{ provide: APP_NAME, useValue: 'Test App!' } | ||
] | ||
}); | ||
inject([FirebaseApp, AngularFireAnalytics], (app_: FirebaseApp, _analytics: AngularFireAnalytics) => { | ||
app = app_; | ||
analytics = _analytics; | ||
})(); | ||
}); | ||
|
||
afterEach(done => { | ||
app.delete(); | ||
done(); | ||
}); | ||
|
||
describe('<constructor>', () => { | ||
|
||
it('should be an AngularFireAuth type', () => { | ||
expect(analytics instanceof AngularFireAnalytics).toEqual(true); | ||
}); | ||
|
||
it('should have the Firebase Functions instance', () => { | ||
expect(analytics.analytics).toBeDefined(); | ||
}); | ||
|
||
}); | ||
|
||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Feiyang1 @hsubox76 what would be the interest level in our adding
firebase_*
parameter handling (firebase_screen_id
,firebase_previous_id
,firebase_screen_class
, etc.) into the JS SDK?I know it breaks the separation of concerns a bit but was non-trivial to get working correctly. It's needed to be able to track user flows in the Firebase Console and BigQuery. The iOS and Android SDKs do this automatically for the developer and their behavior is what I modeled here for Angular. I'll be doing the same treatment for React and Ember routers. I suspect other 3p SDKs will been keen on implementing too.
At the very least we should add information on how to implement in the Firebase docs and add types. FYI @gkaldev @schandel @davideast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a lot of Angular experience so maybe wait for @Feiyang1 to take a look but a lot of this looks very Angular-router-specific. What part/params would be passed to the JS SDK to work with? Do we derive screen_class and previous_id from something or would that be passed in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooking into the router will be framework specific of course, my thought would be to add a
logScreenView(this, name: string, additionalParams?: {})
function which would do all the firebase specific event param computation:firebase_screen_id
,firebase_previous_screen
,firebase_screen_class
,firebase_screen
,firebase_previous_class
,firebase_previous_screen
, andfirebase_previous_id
. They're are all undocumented and having to do these on my own to get the most out of Firebase was unexpected.For instance
firebase_screen_id
was challenging to understand, I couldn't find any resources externally and had to "cheat" and look it up on g3 to find out it's a random INT64 (rather than a hashCode of some kind). I'm not 100% confident I got it right either, hence looping in more DelRel folk.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if these are standard for other Firebase platforms and we can come up with a sort of general interface I think it would make sense for us to bake some of this handling into the JS SDK. Let us know if you want to set up a meeting to talk about what we can do with these and pass along your hard-earned research about these params, maybe after you get some more input from other devrels confirming how they work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, you are asking the JS SDK to keep the state of the previous
screen_view
parameters and attach them to the nextscreen_view
request?Can you share a pointer to how iOS SDK and Android SDK handles it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, basically I'm thinking all the undocumented
firebase_*
attributes that make the console work should be something we take care of.I'll send over email now that I'm back from the holiday and we can hop on GVC if additional clarity is needed. That probably would've been a better place for the discussion anyway. 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My Firebase console is still not showing the information I expect, meaning I did not get this 100% correct yet.