-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Adding AngularFireAnalytics, AngularFireRemoteConfig, and refactoring DI Tokens #2187
Merged
Merged
Changes from 5 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 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
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.