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(analytics): Add types to modular package #7109

Merged
merged 67 commits into from
Sep 4, 2023

Conversation

exaby73
Copy link
Contributor

@exaby73 exaby73 commented May 9, 2023

Description

See title

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

@vercel
Copy link

vercel bot commented May 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 4, 2023 9:38pm
react-native-firebase-next ❌ Failed (Inspect) Sep 4, 2023 9:38pm

@exaby73 exaby73 requested a review from mikehardy May 9, 2023 06:39
@exaby73 exaby73 force-pushed the feat/analytics-modular-types branch 2 times, most recently from 4de5e58 to a46bc54 Compare May 9, 2023 06:42
@exaby73 exaby73 force-pushed the feat/analytics-modular-types branch from a46bc54 to 330bfb0 Compare May 9, 2023 06:43
@exaby73 exaby73 changed the title feat(analytics): Add types to the modular API for analytics feat(analytics): Add types to modular package May 9, 2023
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #7109 (d27ece3) into main (cca7053) will increase coverage by 0.42%.
Report is 25 commits behind head on main.
The diff coverage is n/a.

❗ Current head d27ece3 differs from pull request most recent head 5de86f2. Consider uploading reports for the commit 5de86f2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7109      +/-   ##
============================================
+ Coverage     53.80%   54.21%   +0.42%     
  Complexity      735      735              
============================================
  Files           231      231              
  Lines         11629    11633       +4     
  Branches       1864     1867       +3     
============================================
+ Hits           6256     6306      +50     
+ Misses         5026     4981      -45     
+ Partials        347      346       -1     

@mikehardy mikehardy added the Workflow: Needs Review Pending feedback or review from a maintainer. label May 11, 2023
@github-actions
Copy link

github-actions bot commented Jun 8, 2023

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Jun 8, 2023
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.

Mostly comments + questions, and one stated preference

packages/analytics/lib/modular/index.js Show resolved Hide resolved
packages/analytics/lib/modular/index.js Outdated Show resolved Hide resolved
packages/analytics/lib/index.d.ts Show resolved Hide resolved
packages/analytics/lib/index.js Show resolved Hide resolved
packages/analytics/lib/modular/index.d.ts Outdated Show resolved Hide resolved
@mikehardy mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. and removed Stale Workflow: Needs Review Pending feedback or review from a maintainer. labels Jun 13, 2023
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.

still some tangential spacing changes mixed with the diff, still hoping for a link to authoritative reference for string constants

@mikehardy
Copy link
Collaborator

Appears to have taken conflicts as I added getSessionId support from #6633

Bilal-Abdeen and others added 5 commits July 25, 2023 13:47
Additional modifications to AppDelegate.mm are required in order to load the RNFBAppCheckModule.  Without initializing the AppCheckModule before calling [FIRApp configure], the setup of a custom provider will fail.
@CLAassistant
Copy link

CLAassistant commented Jul 25, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
17 out of 18 committers have signed the CLA.

✅ exaby73
✅ Bilal-Abdeen
✅ aadito123
✅ zhiqingchen
✅ Salakar
✅ ashuvssut
✅ mikehardy
✅ falleco
✅ ibobo
✅ galezra
✅ billykwok
✅ brianpeiris
✅ southorange0929
✅ dlackty
✅ MayoudP
✅ skam22
✅ donaldkwong
❌ dependabot[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

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 is still not ready to go - I saw a couple things I did not see before and there was an unanswered question

The merge commits make the commit change indecipherable in general and the spacing changes make the combined diff difficult to look at but I reviewed it again nonetheless

Please check the new comments and we can get this in

packages/analytics/lib/modular/index.js Outdated Show resolved Hide resolved
packages/analytics/lib/modular/index.js Outdated Show resolved Hide resolved
packages/analytics/lib/index.d.ts Show resolved Hide resolved
mikehardy
mikehardy previously approved these changes Sep 4, 2023
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.

typedefs are used to get clean types in the jsdoc now, Nabeel got the reference to the upstream event names in there, just pending CI

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.

CI went green but I had a copy/paste error in last push, fixed re-pushed and good to go

@mikehardy mikehardy merged commit b6f64bf into main Sep 4, 2023
14 checks passed
@mikehardy mikehardy deleted the feat/analytics-modular-types branch September 4, 2023 21:35
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Sep 4, 2023
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.