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

chore: make ReactNativeFirebaseMessagingSerializer + events builder functions public #4618

Merged
merged 1 commit into from
Nov 26, 2020

Conversation

alexisbronchart
Copy link
Contributor

Description

I am in a situation where I need to manually send events through ReactNativeFirebaseEventEmitter. See discussion #4577
In order to properly do so, I'd like to use the functions present in ReactNativeFirebaseMessagingSerializer.
This PR just adds the public access modifier to that class and to the functions that build events.

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 Nov 25, 2020

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/7h6jneyeo
✅ Preview: Failed

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #4618 (ea972e7) into master (bda2d67) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4618      +/-   ##
==========================================
- Coverage   85.42%   85.37%   -0.05%     
==========================================
  Files         109      109              
  Lines        3710     3710              
  Branches      346      346              
==========================================
- Hits         3169     3167       -2     
- Misses        476      478       +2     
  Partials       65       65              

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.

We have some history of doing this in Firebase classes, and this follows the exact precedent

We may be slightly less careful with regards to breaking changes coming through as this now represents public API surface area that is not normal for us to consider but we'll try to semver those correctly too (if they ever change, unlikely, but just a heads up - it'll be a compile-time thing anyway so will fail fast even if we mess up semver here)

So these LGTM

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

I have a medium-size stack of changes pending release now, plus some CI work to do, which is to say that I'm expecting to release these shortly (I want to get that stack out...) but it may take a day or three (the CI work is also important prior to release, there are a couple things not working well and I don't want to have a quality error). So please have patience but also you can click the "patch package" CI check from this PR run and you'll find an artifact associated with that CI check. It has the official patch-package files you can use in the meantime, knowing that they'll match the future release exactly so you don't have to wait on me

Cheers

@mikehardy
Copy link
Collaborator

CI triage / fix completed in #4600 - this should be good to go

@mikehardy mikehardy merged commit e54fecc into invertase:master Nov 26, 2020
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Nov 26, 2020
hmhm2292 pushed a commit to hmhm2292/react-native-firebase that referenced this pull request Jul 13, 2021
…der APIs public (invertase#4618)

This enables their use in brownfield situations
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