Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
GoodSir42 wants to merge 3 commits into invertase:main
base: main
Choose a base branch
Loading
from GoodSir42:main

Conversation

Copy link

@GoodSir42 GoodSir42 commented Sep 3, 2025

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_name and screen class are used the issue does not exist and screen views are tracked as before via logScreenView

See issue #8609 for more details on this.

It is likely that the EventParams firebase_screen and firebase_screen_class are 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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
    • Other (macOS, web)
  • My change includes tests;
    • [-] e2e tests added or updated in packages/\*\*/e2e
    • [-] jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes (it will require changes to the passed in parameters, but it is more fixing than breaking)
    • [-] No

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 :) 🔥

kashmiry, stetbern, LowellBateman, and Zimmee reacted with thumbs up emoji kolphi-neofonie and LowellBateman reacted with hooray emoji kolphi-neofonie, nakapon9517, and LowellBateman reacted with heart emoji
Copy link

CLAassistant commented Sep 3, 2025
edited
Loading

CLA assistant check
All committers have signed the CLA.

Copy link

vercel bot commented Sep 3, 2025
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
react-native-firebase Ready Ready Preview Comment Sep 17, 2025 11:21am

Copy link
Author

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

jukkamarttinen and LowellBateman reacted with thumbs up emoji

Copy link

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

jukkamarttinen and LowellBateman reacted with thumbs up emoji

Copy link
Collaborator

@MichaelVerdon MichaelVerdon left a comment
edited
Loading

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.

Copy link
Author

GoodSir42 commented Oct 7, 2025
edited
Loading

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

Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelVerdon MichaelVerdon MichaelVerdon approved these changes

+1 more reviewer

@liamjones liamjones liamjones left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[🐛] logEvent versus logScreenView

AltStyle によって変換されたページ (->オリジナル) /