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

fix(analytics): allow custom event parameters for screen_view events #5811

Merged

Conversation

tomonacci
Copy link
Contributor

Description

Fixes the problems described in #4594 by making type checks for logScreenView considerably more lenient. The implementation is according to #4594 (comment):

superstruct apparently supports a way of defining some fixed things (and validating them) while allowing arbitrary other things: https://docs.superstructjs.org/resources/faq#how-can-i-allow-unknown-keys-with-object-structs

If you can bend the definitions of structs.ScreenView such that it passes validation correctly (screen class and name are still checked etc) and takes the extra parameters I will happily merge it

In the FAQ linked above it says to use type instead of object but in the version react-native-firebase depends on type is called interface (it was renamed in 0.10.0 release).

Related issues

Fixes #4594.

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • 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
    • No

Test Plan

I added a test case.


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Oct 25, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/5kp1fLvCJXVPb8Mu9SX5T9D8bvnt
✅ Preview: Canceled

[Deployment for 7358020 canceled]

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/DPMpDoZ6diHiACRxvFTvM3RpCjW2
✅ Preview: https://react-native-firebase-git-fork-tomonacci-scree-898d20-invertase.vercel.app

@CLAassistant
Copy link

CLAassistant commented Oct 25, 2021

CLA assistant check
All committers have signed the CLA.

mikehardy
mikehardy previously approved these changes Oct 25, 2021
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This is really clean and thanks especially for including test support.
Yes our superstruct version is really old, I brought it forward a few notches but there was some awful interaction between current versions and the way we're doing something under the covers so I wasn't able to bring it all the way forward, yet 😩

Happy to merge this though!

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Oct 25, 2021
@mikehardy
Copy link
Collaborator

Assuming this passes tests etc you'll find a patch-package compatible patch set as an artifact on this github action: https://github.com/invertase/react-native-firebase/runs/3996604652?check_suite_focus=true - I usually try to release about once a week near the end of the week so that may be useful for you until it's time to publish this one

@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #5811 (512d449) into master (9f00ac9) will increase coverage by 0.45%.
The diff coverage is 100.00%.

❗ Current head 512d449 differs from pull request most recent head 7358020. Consider uploading reports for the commit 7358020 to get more accurate results

@@             Coverage Diff              @@
##             master    #5811      +/-   ##
============================================
+ Coverage     52.54%   52.98%   +0.45%     
  Complexity      620      620              
============================================
  Files           208      208              
  Lines         10118    10118              
  Branches       1589     1589              
============================================
+ Hits           5315     5360      +45     
+ Misses         4543     4503      -40     
+ Partials        260      255       -5     

@mikehardy
Copy link
Collaborator

Fantastic - merged and will be out in next release, thanks again for this

@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Oct 25, 2021
@tomonacci
Copy link
Contributor Author

Thanks for the quick review and merge @mikehardy!

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

Successfully merging this pull request may close these issues.

[🐛] Analytics - cannot log custom parameters
3 participants