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(app, expo): Update iOS AppDelegate plugin to work with Expo SDK 43 #5796

Merged
merged 3 commits into from
Oct 17, 2021

Conversation

barthap
Copy link
Contributor

@barthap barthap commented Oct 16, 2021

Description

Resolves problem described in #5788

Followed solution from the react-native-maps plugin as suggested in the conversation: #5788 (comment)

I did NOT update other places to use the new syntax (mergeContents()), but I can if asked to do so.

To avoid possible problems with duplicate [FIRApp configure] call after updating, the new plugin skips if the call is found. The mergeContents has this functionality already, but compares only @expo generated headers which are introduced in this PR, so it wouldn't work with previous plugin versions.
These problems themselves could be easily solved by running expo prebuild --clean, but IMO it's better to have an extra check instead of users creating issues "expo plugin stopped working. Firebase default app instance already initialized" etc 😉

Related issues

Resolves #5788

Release Summary

  • Updated iOS Expo config plugin for @react-native-firebase/app to work with Expo SDK 43.

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

  • Added test fixture for new AppDelegate.m
  • Now we have two test cases: for current/old pre-SDK 43 AppDelegate as well as for SDK 43+ (and non-Expo) AppDelegate.
  • Looking at the .snap snapshot files, we can see that the plugin works perfectly in both cases 😁

    I'm glad that we've decided to use snapshot testing technique here - it's really helpful in such cases

@vercel
Copy link

vercel bot commented Oct 16, 2021

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 – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/DAxP4rY9JuTyabR5TLs3qHnJitVo
✅ Preview: https://react-native-firebase-git-fork-barthap-app-plu-ae506c-invertase.vercel.app

react-native-firebase-next – ./website_modular

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

[Deployment for a9e91be canceled]

@codecov
Copy link

codecov bot commented Oct 16, 2021

Codecov Report

Merging #5796 (d55f5bd) into master (9c66f37) will decrease coverage by 0.01%.
The diff coverage is 80.00%.

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

@@             Coverage Diff              @@
##             master    #5796      +/-   ##
============================================
- Coverage     53.14%   53.14%   -0.00%     
  Complexity      620      620              
============================================
  Files           208      208              
  Lines         10069    10071       +2     
  Branches       1542     1542              
============================================
+ Hits           5350     5351       +1     
- Misses         4463     4464       +1     
  Partials        256      256              

@mikehardy
Copy link
Collaborator

Thanks for this!
E2E test failures are obviously unrelated (two known flakes, alas) - I restarted them but they are not a gate here

When you say:

I did NOT update other places to use the new syntax (mergeContents()), but I can if asked to do so.

Does that imply perf and crashlytics will have similar issues? If that understanding is correct, any effort to bring them in line with expo-future-state would be really appreciated. This repo sees enough new users / day that if everything isn't nailed down (code + tests + docs) you can see how quickly issue traffic / response eats up all available time then no forward progress happens. The alternative (everything nailed down) is an actual time-saver then

@barthap
Copy link
Contributor Author

barthap commented Oct 16, 2021

Does that imply perf and crashlytics will have similar issues?

Not really, unless Google changes the way of installing Firebase on Android, or Gradle file structure changes significantly. The only difference is that the new method adds these fancy @expo generated begin ... end comments.

any effort to bring them in line with expo-future-state would be really appreciated.

The expo-future-state of plugin system is still unknown 😉 Maybe soon there will be a more reliable way to make such changes, because both mentioned above still rely on regex.

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.

LGTM - thank you for your continued help here on the expo integration, I really appreciate it and obviously it has some traction from the users based on related traffic

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Oct 16, 2021
@mikehardy mikehardy merged commit d67c3b9 into invertase:master Oct 17, 2021
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Oct 17, 2021
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.

App / Expo plugin - iOS might break with Expo SDK 43
2 participants