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 AppDelegate config plugin for Expo SDK 44 #5940

Merged
merged 4 commits into from
Dec 15, 2021

Conversation

barthap
Copy link
Contributor

@barthap barthap commented Dec 15, 2021

Description

See #5936 (comment)

The AppDelegate template for Expo has changed again.

  • Updated the existing regex
  • Proposed a fallback regex, if the line matched by the previous regex ever changes again (hope not!). It matches for - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:, but surprisingly is less stable than the previous one. Why? It can be multiline (e.g. the method opening bracket { can be in the same or new line). And mergeContents() from @expo/config-plugins doesn't support multiline matchers. 😒 So I left this solution as a second chance.

Are we forced to rely on RegExp forever?

I am leaving it here for further discussion / reference.

Hopefully not. Interestingly, the line that broke SDK 44, actually was part of a change (series of changes), that would allow libraries to define their own "App Delegate Extensions" (I'm still not sure about the official name), which can contain custom code to be executed in AppDelegate methods.

In this case, such extension could look like this (it's Swift, but afaik there's a possibility to do it in Obj-C too):

import ExpoModulesCore
import Firebase

public class RNFBExpoAppDelegate: ExpoAppDelegateSubscriber {
  public func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey : Any]? = nil) -> Bool {
    FirebaseApp.configure()
    return true
  }
}

Then, this class name should be provided in one expo-specific configuration file (expo-module.config.json) shipped with the library, and expo-modules-autolinking will do the rest.

More info in this PR: expo/expo#14867

This will let us get rid of that regex changes. Why didn't I implement it yet and stay with regex? A couple of reasons:

  • It's undocumented yet
  • It's introduced in SDK 44, the API might be unstable.
  • It most likely requires RNFB to add ExpoModulesCore as an iOS pod dependency - I don't want to mess with it here yet.
  • That dependency is a Swift module, but I don't know much about objc-swift interop between libraries

If the above are not blockers, then I can try to implement this.

Related issues

Fixes #5936

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

  • Regex tested in Regex101 (links in code)
  • Tested on an SDK 44 app,
  • Updated jest tests and snapshots

@vercel
Copy link

vercel bot commented Dec 15, 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/7QKDRvfEQHciD9QzGi9b3u1e6A4c
✅ Preview: https://react-native-firebase-git-fork-barthap-barthap-8f13a1-invertase.vercel.app

react-native-firebase-next – ./website_modular

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

[Deployment for 8e3a90b canceled]

@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #5940 (0122e53) into main (fcf375a) will increase coverage by 0.04%.
The diff coverage is 78.95%.

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

@@             Coverage Diff              @@
##               main    #5940      +/-   ##
============================================
+ Coverage     53.01%   53.04%   +0.04%     
  Complexity      622      622              
============================================
  Files           208      208              
  Lines         10160    10171      +11     
  Branches       1614     1618       +4     
============================================
+ Hits           5385     5394       +9     
- Misses         4521     4523       +2     
  Partials        254      254              

mikehardy
mikehardy previously approved these changes Dec 15, 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.

I have a strong opinion against adding any Pods that are not pointed directly at critical upstream dependencies, e.g. to google firebase pods, so I do not want to pursue that.

I have a second strong opinion that they should base off the raw react-native template so that we are all working from the same base and it would be "stable" since that does not change with much frequency at all. I do a lot of dynamic app construction via script and it works great, I don't know why they are not just sticking to the upstream and "borrowing" it's stability?

This works for me though, with fallback also, that's fine - if you like the comment change, and this gets us going for Expo 44 I'm fine leaving it at that.

I appreciate the continued attention here though - relatively trifling opinions on code aside, I have 10x strong positive opinions of anyone that takes the time to contribute back, thanks @barthap

@mikehardy mikehardy changed the title [expo] Fix AppDelegate plugin for Expo SDK 44 fix(app, expo): Update AppDelegate config plugin for Expo SDK 44 Dec 15, 2021
Co-authored-by: Mike Hardy <[email protected]>
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.

Up to you I think, looks good to me and if you thought it was ready for review (not draft) we could merge as is and release

@barthap
Copy link
Contributor Author

barthap commented Dec 15, 2021

Hah, I marked this as ready moments ago, forgot to click it earlier, sorry 😉

P.S. Thanks for fixing the title!

@mikehardy mikehardy merged commit 185756d into invertase:main Dec 15, 2021
@nandorojo
Copy link
Contributor

Is this released btw? Thanks!

@barthap
Copy link
Contributor Author

barthap commented Dec 16, 2021

Yep, 14.0.1

@mikehardy
Copy link
Collaborator

@nandorojo -> https://invertase.io/blog/react-native-firebase-versioning --> https://github.com/invertase/react-native-firebase/blob/main/CHANGELOG.md for a happy library user, note those

@barthap barthap deleted the @barthap/expo-sdk44/app-delegate branch June 28, 2022 17:22
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.

[🐛] Config Plugin breaks on Expo SDK 44
3 participants