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

feat(perf): support "perf_auto_collection_enabled" flag in firebase.json #4870

Merged
merged 1 commit into from
Feb 4, 2021
Merged

Conversation

michaelgmcd
Copy link
Contributor

@michaelgmcd michaelgmcd commented Feb 4, 2021

Description

More or less clones 9a24ecd

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 - Theoretically if someone had this flag set beforehand, it would not have been accurately reflected. This could be a breaking change in the sense that it would change someone's app configuration.
    • No

Test Plan


Checking that firebase performance is disabled on app start when this flag is set.

@vercel
Copy link

vercel bot commented Feb 4, 2021

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

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/dfu7b17qf
✅ Preview: https://react-native-firebase-git-fork-michaelgmcd-master.invertase.vercel.app

@CLAassistant
Copy link

CLAassistant commented Feb 4, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #4870 (ead02a4) into master (9c4ada8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4870   +/-   ##
=======================================
  Coverage   89.09%   89.09%           
=======================================
  Files         109      109           
  Lines        3718     3718           
  Branches      347      347           
=======================================
  Hits         3312     3312           
  Misses        364      364           
  Partials       42       42           

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.

This looks in general fantastic, thank you!
I had one change I propose - with a question attached - that I'd like your feedback on

packages/perf/android/build.gradle Outdated Show resolved Hide resolved
@michaelgmcd michaelgmcd changed the title feature: support "perf_auto_collection_enabled" flag in firebase.json improvement: support "perf_auto_collection_enabled" flag in firebase.json Feb 4, 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.

This looks great - thank you!

I'm a little bit back-logged on publishing a new version but 10.6.0 with this change in it should release "really soon now" (a day? three? less than a week)

@mikehardy mikehardy changed the title improvement: support "perf_auto_collection_enabled" flag in firebase.json feat(perf): support "perf_auto_collection_enabled" flag in firebase.json Feb 4, 2021
@mikehardy mikehardy merged commit e54bf49 into invertase:master Feb 4, 2021
@michaelgmcd
Copy link
Contributor Author

All good. We'll use a workaround until it's ready. Thanks for the quick review!

@mikehardy
Copy link
Collaborator

I had a moment and fixed the bottleneck problem #4708 and got 10.6.1 out with all peerDependencies between the modules correct and everything 😅 - good to go!

androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Aug 9, 2021
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.

3 participants