invertase / react-native-google-mobile-ads

React Native Google Mobile Ads enables you to monetize your app with AdMob.
https://docs.page/invertase/react-native-google-mobile-ads
Other
688 stars 139 forks source link

[🐛] The library breaks builds for Mac Catalyst #354

Closed birdofpreyru closed 1 year ago

birdofpreyru commented 1 year ago

What happened?

I've tried to build an existing Android/iOS RN app using rn-google-mobile-ads for Mac Catalyst target, and the build fails, because Google's Mobile Ads SDK does not support Mac Catalyst. However, I checked that if I comment out everything inside this library's native code, beside the high-level interface exposed to RN's JS layer, and also comment out the links to Mobile Ads SDK in this library's .podspec file — then Mac Catalyst build works fine (well, after fixing a bunch of minor issues in my other dependencies), and my app works fine at the runtime, at the first glance. I mean, it obviously does not show any ads, but it does not crash, beside no changes in my code, thus still doing all calls to rn-google-mobile-ads the same way it does on other platforms. It also still shows fallback ads (in my app I use banners, and I use interstitials, so normally if a banner or interstitial is not loaded the app shows a dummy ad in that place, inviting users to subscribe, to remove ads, and giving them links to the subscriptions page) — thus, effectively, it works quite good for me.

Hence, my suggestion is to add a bunch of #ifndef TARGET_OS_MACCATALYST pragmas into ios code, and whatever is the equivalent into .podspec, to ensure that when build for Mac Catalyst the library does not break the build, and does not crash at runtime, just does nothing when its functions are called.

What do you think about it @mikehardy ? Am I missing an alternative way to conditionally include / exclude RN library into an app, depending on the build target?

Platforms

Only on iOS

React Native Info

irrelevant

Are your using Typescript?

package.json

irrelevant

app.json

irrelevant

ios/Podfile

No response

android/build.gradle

No response

android/app/build.gradle

No response

android/settings.gradle

No response

AndroidManifest.xml

No response

mikehardy commented 1 year ago

That would be a functional workaround for lack of macCatalyst support in the SDK, I'd take a PR like that, sure the IFDEF path around things that don't work is the only way I've found to do it historically, but I've never had to fiddle with the podspec making it conditional as well. That should be possible though? Anyway, if you're able to get anything going that's a podspec conditional + some IFDEFs I think that's about as good as it gets and I'd merge

github-actions[bot] commented 1 year ago

Hello 👋, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.

birdofpreyru commented 1 year ago

Isn't stale :)

birdofpreyru commented 1 year ago

For a record, if anybody else is interested in making this work, in my fork of the library I did initial changes in this branch: https://github.com/birdofpreyru/react-native-google-mobile-ads/tree/mac-catalyst

With these, provided that pods were installed with MAC_CATALYST environment variable set (i. e. like MAC_CATALYST=1 pod install), the library builds for Mac Catalyst within my project. However, I was not able to test it yet, as my project depends on other libraries (e.g. RN Firebase) failing to build for Mac Catalyst; and I am also not able to build & run the example app in this library, not without spending more time than I have available for this now.

birdofpreyru commented 1 year ago

Hey @mikehardy , how are you doing? I've just created a PR #389 for this issue, which both solves the problem for me for Mac Catalyst builds, and also does not break anything for normal iOS builds. Do you think we can merge it?

github-actions[bot] commented 1 year ago

Hello 👋, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.

mikehardy commented 1 year ago

:tada: This issue has been resolved in version 11.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: