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(ios, messaging): add ios version guard for UNAuthorizationOptionProvidesAppNotificationSettings usage #5986

Merged
merged 3 commits into from
Jan 4, 2022

Conversation

matinzd
Copy link
Contributor

@matinzd matinzd commented Jan 4, 2022

Description

There are some warnings while using UNAuthorizationOptionProvidesAppNotificationSettings without version check.

Related issues

No related issues.

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

Test Plan


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

Add check for `UNAuthorizationOptionProvidesAppNotificationSettings` in iOS 12 and newer
Add check for `UNAuthorizationOptionProvidesAppNotificationSettings`
@vercel
Copy link

vercel bot commented Jan 4, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/F2hJiyFXzUK11jb9LJYq3jXCvwwB
✅ Preview: Canceled

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/Db87sRicvDUBh3vGBix97Y142SAS
✅ Preview: https://react-native-firebase-git-fork-matinzd-main-invertase.vercel.app

@CLAassistant
Copy link

CLAassistant commented Jan 4, 2022

CLA assistant check
All committers have signed the CLA.

@mikehardy mikehardy changed the title Add check for UNAuthorizationOptionProvidesAppNotificationSettings in iOS 12 and newer lint(messaging, ios): add ios version guard for UNAuthorizationOptionProvidesAppNotificationSettings usage Jan 4, 2022
@mikehardy mikehardy changed the title lint(messaging, ios): add ios version guard for UNAuthorizationOptionProvidesAppNotificationSettings usage style(lint, messaging): add ios version guard for UNAuthorizationOptionProvidesAppNotificationSettings usage Jan 4, 2022
@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #5986 (f968f0b) into main (8015779) will decrease coverage by 0.37%.
The diff coverage is n/a.

❗ Current head f968f0b differs from pull request most recent head 8bb0469. Consider uploading reports for the commit 8bb0469 to get more accurate results

@@             Coverage Diff              @@
##               main    #5986      +/-   ##
============================================
- Coverage     52.95%   52.59%   -0.36%     
  Complexity      622      622              
============================================
  Files           208      208              
  Lines         10189    10189              
  Branches       1619     1619              
============================================
- Hits           5395     5358      -37     
- Misses         4540     4574      +34     
- Partials        254      257       +3     

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.

Thanks for this! At first I thought this was just a lint-type thing but this is actually important to fix possible crash on ios 11 devices, so I'll merge and release it as soon as CI finishes digesting the change.

Ironically I had started work on making these errors much easier to see with #5935 since I noticed this warning myself but it was lost in the noise, and it was on my list of things to clean but you beat me to it :-). I'll sweep the rest of the warnings shortly and RNFB will build quietly again

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Jan 4, 2022
@mikehardy mikehardy changed the title style(lint, messaging): add ios version guard for UNAuthorizationOptionProvidesAppNotificationSettings usage fix(ios, messaging): add ios version guard for UNAuthorizationOptionProvidesAppNotificationSettings usage Jan 4, 2022
@matinzd
Copy link
Contributor Author

matinzd commented Jan 4, 2022

Thanks for this! At first I thought this was just a lint-type thing but this is actually important to fix possible crash on ios 11 devices, so I'll merge and release it as soon as CI finishes digesting the change.

Ironically I had started work on making these errors much easier to see with #5935 since I noticed this warning myself but it was lost in the noise, and it was on my list of things to clean but you beat me to it :-). I'll sweep the rest of the warnings shortly and RNFB will build quietly again

There are some problems while running ios e2e (timeout). I don't think that would be an issue.

@mikehardy
Copy link
Collaborator

Indeed - that's a flaky test which has been a pain for me the last few weeks, I've got a branch going right now locally where I will clean that up. I agree with you that the timeout on that test (storage.getDownloadUrl) should have nothing to do with your messaging change, further the app built (so compile works) and already ran through the e2e tests for the messaging module (they are alphabetical) prior to getting to storage.

So I'm comfortable merging this. Thanks again!

@mikehardy mikehardy merged commit e8922c0 into invertase:main Jan 4, 2022
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Jan 4, 2022
@matinzd
Copy link
Contributor Author

matinzd commented Jan 7, 2022

Indeed - that's a flaky test which has been a pain for me the last few weeks, I've got a branch going right now locally where I will clean that up. I agree with you that the timeout on that test (storage.getDownloadUrl) should have nothing to do with your messaging change, further the app built (so compile works) and already ran through the e2e tests for the messaging module (they are alphabetical) prior to getting to storage.

So I'm comfortable merging this. Thanks again!

You're welcome! Yeah, I know it's much of a pain for me as well to deal with e2e tests in our CI.

@mikehardy
Copy link
Collaborator

Nailed that flaky test though :-) 454dc70 - honestly you (as a contributor doing the awesome thing by submitting a PR) getting hung up on it was the final straw. I want the PR experience to be clean + easy 😅 - so at least for future PRs, and at least for now, our CI is all green first time. So I suppose I owe you thanks not just for the PR but for motivation to make the whole thing better. Cheers

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