fix: update Expo plugin to be compatible with objcpp #6223

kbrandwijk · 2022-04-28T08:56:39Z

Description

Expo SDK 45, which is now in beta, has switched to using objcpp for AppDelegate on iOS. The current import syntax, using @import, is incompatible with this. Therefore, SDK 45 beta users using the config plugin are experiencing build issues. This change is backwards compatible with cpp, Firebase switched to it 2 years ago (firebase/FirebaseUI-iOS#875 and the linked PR in that issue).

Related issues

Fixes #6221

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:

👉 Star this repo on GitHub ⭐️
👉 Follow React Native Firebase and Invertase on Twitter

vercel · 2022-04-28T08:56:44Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
react-native-firebase	✅ Ready (Inspect)	Visit Preview	Apr 28, 2022 at 0:22AM (UTC)

1 Ignored Deployment

Name	Status	Preview	Updated
react-native-firebase-next	⬜️ Ignored (Inspect)		Apr 28, 2022 at 0:22AM (UTC)

CLAassistant · 2022-04-28T09:12:52Z

All committers have signed the CLA.

barthap

Looks good, thanks 😁 I missed that change in my PR.

barthap · 2022-04-28T12:20:15Z

packages/app/plugin/src/ios/appDelegate.ts

@@ -19,7 +19,7 @@ export function modifyObjcAppDelegate(contents: string): string {
    contents = contents.replace(
      /#import "AppDelegate.h"/g,
      `#import "AppDelegate.h"
-@import Firebase;`,
+#import <Firebase/Firebase.h>`,


According to the RNFB docs, I don't know if it shouldn't be #import <Firebase.h> 🤔

I believe we use to advice using @import Firebase a long time ago, but we also switched, maybe that's the confusion?

Either way this reminded me of some previous issues, and in triage of those we made sure both ways work (or, worked - @import won't work now I understand that:
#4069

Also, I made this change myself recently in my demo script - which hadn't been updated from @import style yet, and I can say I know this is working as well with new style:
mikehardy/rnfbdemo@cfed4d8#diff-3e27ae623577d0c559bd8ab973a0c34af8cc65ef9beb9868a9cdb029854f8078R33

Which is all to say: I'm pretty comfortable this will be backwards compatible (excellent!) and also I know for RN68 you need it as well

mikehardy

This looks great! And yes, I discovered it was needed as well, when I updated my demo script to the RN68 builds, but didn't think of the expo plugins here :-). Thanks for posting this, and sorry about the CLA-bot hassle - who knows what happened there
I'll push a patch release in a moment

mikehardy · 2022-04-28T12:43:18Z

packages/app/plugin/src/ios/appDelegate.ts

@@ -19,7 +19,7 @@ export function modifyObjcAppDelegate(contents: string): string {
    contents = contents.replace(
      /#import "AppDelegate.h"/g,
      `#import "AppDelegate.h"
-@import Firebase;`,
+#import <Firebase/Firebase.h>`,


I believe we use to advice using @import Firebase a long time ago, but we also switched, maybe that's the confusion?

Either way this reminded me of some previous issues, and in triage of those we made sure both ways work (or, worked - @import won't work now I understand that:
#4069

Also, I made this change myself recently in my demo script - which hadn't been updated from @import style yet, and I can say I know this is working as well with new style:
mikehardy/rnfbdemo@cfed4d8#diff-3e27ae623577d0c559bd8ab973a0c34af8cc65ef9beb9868a9cdb029854f8078R33

Which is all to say: I'm pretty comfortable this will be backwards compatible (excellent!) and also I know for RN68 you need it as well

codecov · 2022-04-28T12:47:13Z

Codecov Report

Merging #6223 (cd07ad6) into main (0fd4b4f) will decrease coverage by 27.60%.
The diff coverage is n/a.

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

@@              Coverage Diff              @@
##               main    #6223       +/-   ##
=============================================
- Coverage     53.40%   25.80%   -27.59%     
=============================================
  Files           208       97      -111     
  Lines         10302     4283     -6019     
  Branches       1633     1046      -587     
=============================================
- Hits           5501     1105     -4396     
+ Misses         4543     2579     -1964     
- Partials        258      599      +341

mikehardy · 2022-04-28T12:51:38Z

I'm short-cutting some of the CI steps because I just ran through everything and did a release yesterday, plus I know which aspects could/would change based on these file edits. So I've merged it after seeing the affected jobs complete and likewise I know what's affected on main (nothing, vs this PR) so I just kicked off the publish workflow. Should have a patch version up on npmjs.com in a few minutes. Cheers!

nandorojo · 2022-04-28T18:58:39Z

I was just about to open an issue, thank you for this!

* Update setup-native-firebase.md After doing some research, I came across [this](invertase/react-native-firebase#6223) Pull Request, suggesting to instead import Firebase with `#import <Firebase/Firebase.h>` as opposed to `@import Firebase;` for SDK 45.

[app] update Expo plugin to be compatible with objcpp

ee1eaf9

kbrandwijk added 2 commits April 28, 2022 10:59

test: update test snapshot to match the change

de70711

test: update additional snapshots

107bc26

kbrandwijk changed the title ~~[app] update Expo plugin to be compatible with objcpp~~ fix: update Expo plugin to be compatible with objcpp Apr 28, 2022

fix: update import statements properly this time

0348997

vercel bot deployed to Preview – react-native-firebase April 28, 2022 09:15 View deployment

This comment was marked as resolved.

Sign in to view

cdedreuille mentioned this pull request Apr 28, 2022

[🐛] Expo SDK 45 error - Can't compile AppDelegate.mm [Fixed in 14.9.1 ✨ ] #6221

Closed

10 tasks

fix: remove superfluous semicolon

4269c8e

barthap approved these changes Apr 28, 2022

View reviewed changes

vercel bot deployed to Preview – react-native-firebase April 28, 2022 12:22 View deployment

invertase deleted a comment from CLAassistant Apr 28, 2022

mikehardy approved these changes Apr 28, 2022

View reviewed changes

mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Apr 28, 2022

mikehardy merged commit 9de82d3 into invertase:main Apr 28, 2022

mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Apr 28, 2022

darrylyoung mentioned this pull request May 9, 2022

Update setup-native-firebase.md expo/expo#17405

Merged

This was referenced Jun 14, 2022

Module imports need to be removed for Objective-C++ compatibility (React Native 0.68+) segmentio/analytics-ios#1036

Open

Objective-C modules are incompatible with Objective-C++ (React Native 0.68+) Iterable/iterable-swift-sdk#563

Closed

DoctorJohn mentioned this pull request Sep 3, 2022

fix(expo, ios): expo plugin added import multiple times #6507

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update Expo plugin to be compatible with objcpp #6223

fix: update Expo plugin to be compatible with objcpp #6223

kbrandwijk commented Apr 28, 2022 •

edited

Loading

vercel bot commented Apr 28, 2022 •

edited

Loading

CLAassistant commented Apr 28, 2022 •

edited

Loading

This comment was marked as resolved.

barthap left a comment

barthap Apr 28, 2022

mikehardy Apr 28, 2022

mikehardy left a comment

mikehardy Apr 28, 2022

codecov bot commented Apr 28, 2022

mikehardy commented Apr 28, 2022

nandorojo commented Apr 28, 2022

fix: update Expo plugin to be compatible with objcpp #6223

fix: update Expo plugin to be compatible with objcpp #6223

Conversation

kbrandwijk commented Apr 28, 2022 • edited Loading

Description

Related issues

Release Summary

Checklist

Test Plan

vercel bot commented Apr 28, 2022 • edited Loading

CLAassistant commented Apr 28, 2022 • edited Loading

This comment was marked as resolved.

barthap left a comment

Choose a reason for hiding this comment

barthap Apr 28, 2022

Choose a reason for hiding this comment

mikehardy Apr 28, 2022

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy Apr 28, 2022

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2022

Codecov Report

mikehardy commented Apr 28, 2022

nandorojo commented Apr 28, 2022

kbrandwijk commented Apr 28, 2022 •

edited

Loading

vercel bot commented Apr 28, 2022 •

edited

Loading

CLAassistant commented Apr 28, 2022 •

edited

Loading