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

[Wallet] Fix Firebase regressions on iOS #3650

Merged
merged 4 commits into from
Apr 29, 2020
Merged

Conversation

jeanregisser
Copy link
Contributor

@jeanregisser jeanregisser commented Apr 29, 2020

Description

Follow up PR to #3320

This PR fixes crashes on iOS related to the database module.
I had already patched those for react-native-firebase v5 (see invertase/react-native-firebase#2734).
Turns out a similar patch is still needed for v6. I will submit a PR upstream too for v6.

Also fixed broken push registration on iOS. There was a new call needed with v6.
I upgraded react-native-firebase to v6.7.1 since it changed push registration too, this way we're already on top again (but for how long? 😉).

These issues are reproducible only when using a real device.

Tested

  • App doesn't crash anymore on iOS in release mode.
  • Push registration works

I could NOT test push being received successfully as notification-service currently has a similar issue as blockchain-api did a couple of days ago (fixed in #3583).

Related issues

Backwards compatibility

Yes.

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #3650 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3650      +/-   ##
==========================================
- Coverage   75.53%   75.53%   -0.01%     
==========================================
  Files         619      619              
  Lines       15789    15791       +2     
  Branches     1724     1793      +69     
==========================================
+ Hits        11926    11927       +1     
+ Misses       3491     3488       -3     
- Partials      372      376       +4     
Flag Coverage Δ
#mobile 75.45% <100.00%> (-0.01%) ⬇️
#web 75.63% <ø> (ø)
Impacted Files Coverage Δ
packages/mobile/src/firebase/firebase.ts 46.00% <100.00%> (+0.54%) ⬆️
...kages/mobile/src/exchange/CeloGoldHistoryChart.tsx 84.31% <0.00%> (-0.84%) ⬇️
packages/web/src/utils/utils.ts 79.31% <0.00%> (ø)
packages/web/src/dev/ValidatorsList.tsx 67.74% <0.00%> (ø)
packages/web/src/utils/abortableFetch.ts 71.42% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb56666...21ccb40. Read the comment docs.

@jeanregisser jeanregisser marked this pull request as ready for review April 29, 2020 16:59
@jeanregisser jeanregisser requested a review from a team April 29, 2020 17:33
@@ -0,0 +1,5 @@
{
"react-native": {
"messaging_ios_auto_register_for_remote_messages": false
Copy link
Contributor Author

@jeanregisser jeanregisser Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we don't want the system popup with asking for push permissions right when the user runs the app for the first time.
Push permission is asked later on when we create the user account.

Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a tricky one, thanks for looking into it!

@jeanregisser jeanregisser added the automerge Have PR merge automatically when checks pass label Apr 29, 2020
@mergify mergify bot merged commit 96eadd6 into master Apr 29, 2020
@mergify mergify bot deleted the jeanregisser/fix-firebase-ios branch April 29, 2020 20:29
jeanregisser added a commit that referenced this pull request Jul 23, 2020
This broke in #3650
because yield call return types are not inferred yet :/
mergify bot pushed a commit that referenced this pull request Jul 23, 2020
### Description

This broke in #3650 with the upgrade of `react-native-firebase`
because `yield call(...)` return types are not inferred yet 😞 

Looks like the device I used when I worked on #3650 was already authorised for push that's why I missed that 2nd regression 🤦 

### Tested

Tested that push permission is being asked again on iOS.

### Backwards compatibility

Yes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
2 participants