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(functions): support custom domains #4950

Merged
merged 12 commits into from
Mar 3, 2021

Conversation

dackers86
Copy link
Member

@dackers86 dackers86 commented Feb 25, 2021

Description

Native Sdks have been updated to allow custom domains to be passed when calling functions. This update allows RNF to allow either custom domains or a region to be provided

Related issues

Fixes #4919

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

BREAKING CHANGE: SDK min required version bump is required for this feature.

Test Plan


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

🔥

@vercel
Copy link

vercel bot commented Feb 25, 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/CyFk1HREYt98TdLrhjDfxDUYbUiH
✅ Preview: https://react-native-f-git-dackers86-added-functions-custom-url-6533de.vercel.app

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #4950 (cf0a1de) into master (bc893ab) will decrease coverage by 0.23%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4950      +/-   ##
==========================================
- Coverage   89.21%   88.99%   -0.22%     
==========================================
  Files         105      109       +4     
  Lines        3482     3721     +239     
  Branches        0      348     +348     
==========================================
+ Hits         3106     3311     +205     
+ Misses        376      369       -7     
- Partials        0       41      +41     

@mikehardy
Copy link
Collaborator

Release notes - should be breaking, and in commit message min version called out:

suggest this as is terse but complete:

BREAKING(functions): add custom domains requires firebase-ios-sdk 7.1.0+ / firebase-android-sdk 26.2.0+

This is important because there are definitely some issues outstanding where people have only seen success using 26.0.0 or 26.1.0 so to them this is definitely a breaking change at our level as compile will fail on missing symbols if the SDK is not updated even though it is a minor for the native SDKs themselves

I don't see any Java implementation change but on review of the underlying PR firebase/firebase-android-sdk#2100 it appears the API there was fully backwards compatible - verified working there?

@mikehardy
Copy link
Collaborator

ios E2E CI currently failing on known issue with pending resolution, reference #4951


[!] CocoaPods could not find compatible versions for pod "FirebaseFirestore":
  In Podfile:
    FirebaseFirestore (from `https://github.com/invertase/firestore-ios-sdk-frameworks.git`)

    RNFBFirestore (from `../../packages/firestore`) was resolved to 10.8.1, which depends on
      Firebase/Firestore (~> 7.6.0) was resolved to 7.6.0, which depends on
        FirebaseFirestore (~> 7.6.0)
error Command failed with exit code 1.

I'm on that one next / immediately

@dackers86 dackers86 changed the title feat(functions): added custom domains BREAKING(functions): add custom domains requires firebase-ios-sdk 7.1.0+ / firebase-android-sdk 26.2.0+ Feb 25, 2021
@dackers86
Copy link
Member Author

dackers86 commented Feb 25, 2021

@mikehardy Yes, test cases added and working for both Android and iOS.

Android reference shown here https://firebase.google.com/docs/reference/android/com/google/firebase/functions/FirebaseFunctions#public-static-firebasefunctions-getinstance-string-regionorcustomdomain

@mikehardy
Copy link
Collaborator

@dackers86 as soon as #4953 looks good to you and passes CI I'll merge it, then once you rebase or merge master in the iOS E2E CI here should pass 🤞

@mikehardy
Copy link
Collaborator

Hmm - the PR Title check failure is unfortunate, I thought a 'BREAKING' would work.

The other alternative - which I worked in the past - is a 'feat' PR title / first line and the commit message can have a BREAKING CHANGE: note at the end, which is more of a squash-merge maintainer note (must do it during the actual merge process)

For extra developer friendliness I usually put BREAKING in top line too like feat(function): add custom domain BREAKING requires etc etc.

example 70974d4 generated changelog https://github.com/invertase/react-native-firebase/blob/master/packages/analytics/CHANGELOG.md#800-2020-11-10

This seems maybe excessively detail oriented but if not done well there will definitely be issues logged later, best to avoid.

@mikehardy mikehardy changed the title BREAKING(functions): add custom domains requires firebase-ios-sdk 7.1.0+ / firebase-android-sdk 26.2.0+ feat(functions): add custom domains BREAKING requires firebase-ios-sdk 7.1.0+ / firebase-android-sdk 26.2.0+ Feb 25, 2021
@Salakar Salakar changed the title feat(functions): add custom domains BREAKING requires firebase-ios-sdk 7.1.0+ / firebase-android-sdk 26.2.0+ feat(functions): add custom domains Feb 25, 2021
@Salakar Salakar changed the title feat(functions): add custom domains feat(functions): support custom domains Feb 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.

LFantasticTM

@mikehardy mikehardy requested a review from Salakar March 2, 2021 13:41
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.

Just re-scanned with special attention to @Salakar comments to verify they were all fixed. This still LGTM and other comments are verified addressed. I'm running tests locally and checking CI now then I'll release this. Chapeau!

@mikehardy mikehardy merged commit 381eae5 into master Mar 3, 2021
@mikehardy
Copy link
Collaborator

mikehardy commented Mar 3, 2021

Yes! The fancy commit message worked: https://github.com/invertase/react-native-firebase/blob/master/CHANGELOG.md#1100-2021-03-03

correctly versioned it as a major, and got the changelog I thought it would.

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.

[🐛] Functions SDK Inconsistency
3 participants