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(template): add Firebase configure to AppDelegate.m #3029

Closed
wants to merge 1 commit into from

Conversation

uguraktas
Copy link

@uguraktas uguraktas commented Dec 21, 2019

if not this code block, Application rebuilds every time it goes to background

Summary

Checklist

  • Supports Android
  • Supports iOS
  • e2e tests added or updated in packages/**/e2e
  • Flow types updated
  • Typescript types updated

Test Plan

Release Plan

[CATEGORY][type] [LOCATION] - Message


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

if not this code block, Application rebuilds every time it goes to background
@codecov
Copy link

codecov bot commented Dec 21, 2019

Codecov Report

Merging #3029 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3029   +/-   ##
=======================================
  Coverage   89.83%   89.83%           
=======================================
  Files         109      109           
  Lines        3381     3381           
=======================================
  Hits         3037     3037           
  Misses        344      344

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.

@Salakar this seems like it matches my memory of the documentation for brownfield projects, is it an oversight in the template?

@Salakar
Copy link
Member

Salakar commented Dec 30, 2019

If I recall correctly, the main reason its not there by default is that iOS will crash if you don't yet have a GoogleService plist file added and then try to init Firebase via this configure method. Plist file is not required for JS side initialised Firebase apps.

Would need to confirm the crash still occurs or not before merging this I think

@Salakar Salakar added the Workflow: Needs Review Pending feedback or review from a maintainer. label Jan 20, 2020
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Salakar Salakar changed the title Add Firebase to AppDelegate.m fix(template): add Firebase configure to AppDelegate.m Mar 25, 2020
@russellwheatley
Copy link
Member

As @Salakar said, this still crashes the app unfortunately.

@russellwheatley russellwheatley added impact: crash Behaviour causing app to crash. blocked: do-not-merge Do not merge this issue without approval by the person who labelled this issue as Do Not Merge and removed Workflow: Needs Review Pending feedback or review from a maintainer. labels Mar 26, 2020
@Salakar
Copy link
Member

Salakar commented May 29, 2020

Closing this as we're not actively maintaining the template anymore as integrating RNFB into existing projects is fairly minimal now so the template doesn't do much in the way of time saving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked: do-not-merge Do not merge this issue without approval by the person who labelled this issue as Do Not Merge impact: crash Behaviour causing app to crash. platform: ios Template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants