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(agp): do not apply namespace if its not available #70

Closed
wants to merge 1 commit into from

Conversation

osamaqarem
Copy link
Owner

@osamaqarem osamaqarem requested a review from KrzysztofMoch June 26, 2023 07:00
@KrzysztofMoch
Copy link
Collaborator

I think we should add back package="com.reactnativeimagecolors" in android manifest to support older versions

@KrzysztofMoch
Copy link
Collaborator

Also should we try implement this solution ? Example here

@osamaqarem
Copy link
Owner Author

yeah we should add it back, thanks for catching that. but i'm not sure about conditionally taking it out too via code - it might be ignored react-native-community/discussions-and-proposals#671 (comment) but I need to test it.

@atlj
Copy link

atlj commented Jun 27, 2023

@osamaqarem Please see the following comment as well: react-native-community/discussions-and-proposals#671 (comment)

@KrzysztofMoch
Copy link
Collaborator

@osamaqarem can i complete this PR or you have already changes on local ?

@osamaqarem
Copy link
Owner Author

I'm thinking to close this PR as integration of expo-modules-core requires AGP 7 minimum, which means if you're on RN67 (AGP 4) you had to upgrade your AGP to 7 to use expo-modules-core in the first place.

In other words, if we were never compatible with AGP 4 (RN67 and below) then we never broke backward compatibility.

I couldn't find docs that say this, but there is this comment on a related issue
expo/expo#17862 (comment)

And then implicitly by expo-modules-core manual integration steps (setting AGP to 7.4):
https://docs.expo.dev/bare/installing-expo-modules/

Lastly, there is guide on from expo about changes needed for custom expo modules about those Gradle/AGP changes, and they don't address the concern of backward compatibility.
https://github.com/expo/fyi/blob/main/expo-modules-gradle8-migration.md

@KrzysztofMoch
Copy link
Collaborator

Okay, sounds reasonable

@osamaqarem
Copy link
Owner Author

I did some testing, and our v2.0.0 release only works with RN>0.70 which matches up to Expo SDK 47
https://docs.expo.dev/versions/latest/#support-for-android-and-ios-versions

This makes sense as per our README for Expo we specify SDK 47 as the minimum. I'll close this PR and update the README mentioning the missing requirement of > RN70 for bare apps, and i'll suggest v1 for older users.

@osamaqarem osamaqarem closed this Jun 29, 2023
@KrzysztofMoch KrzysztofMoch deleted the fix-backward-compat-agp branch June 29, 2023 16:11
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