-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(analytics): fix the typing for screen_view event logging #8687
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
it would be great to get some movement into this rather minor change :) so far @liamjones had a look, but we need another person to review it |
Can we expedite this one please, it will help us a lot, tnx :) |
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.
LGTM thanks for making this:
Are we able to get it fully matching https://firebase.google.com/docs/analytics/screenviews#web ??
Our aim on React Native Firebase is to make it match the js-sdk so users if not looking at react native docs can also refer to js.
unfortunately not, as those property names are illegal for the native SDKs. I also thought about exposing the names like on the JS SDK but then change them internally, but I am a bit lost in the js code there |
|
The only possibility to keep the invalid parameter names I see right now is to have an if in the logEvent function that rebuilds the parameters. If you have any better hints I would gladly implement them. @MichaelVerdon |
|
any news on this? |
|
yes. you MUST NOT use the names starting with If you want to ensure you get no warning in prod you need to specify both firebase_screen_class and firebase_screen. But this will break your screen tracking. Something like this will throw a TS warning but work:
|
|
@AdamGerthel Beyond what @GoodSir42 said, you can check the events as they are picked up by Google Analytics by using DebugView. You'll then be able to see errors in the events like I screenshotted above: #8687 (comment) |

Description
Motivation:
Since migrating from logScreenView to logging screen_view events we lost the screen tracking in our project. To prevent this from happening to other developers only the typing information has to be fixed.
Without the change developers are encouraged to use faulty parameter names, leading to errors in the firebase backend and dropped events due to reserved property names. If the names
screen_nameandscreen classare used the issue does not exist and screen views are tracked as before via logScreenViewSee issue #8609 for more details on this.
It is likely that the EventParams
firebase_screenandfirebase_screen_classare not needed any more.Related issues
Fixes #8609
Release Summary
This changes the names you need to supply to logEvent(analytics, 'screen_view') to what it should have been already. Without a change to parameter names your screen tracking will not work!
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
I only changed typings and validated the change in my own Project since it requires checks on the firebase debugging backend.
And since I read the whole guide I am allowed to add some flames in the end :) 🔥