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

support Android 27 with Gradle v4 + Android plugin v3 #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lampietti
Copy link

No description provided.

@phillbaker
Copy link
Collaborator

@lampietti thanks for the PR! Sorry for the slow turnaround on a review. For some context, can you describe the issue you're solving with this PR? Was this package not working with the rest of your app? Was this generated automatically?

@lampietti
Copy link
Author

@phillbaker This PR solves the issue when you launch an assembleRelease if your project is configured with Gradle v4 and Android Gradle v3.1 (using Android build-tools 27), aapt fails on the :react-native-fingerprint-scanner:verifyReleaseResources when searching for some resources linked on Android 25...

PS: I reworked my PR to manage the ext variables from root project as suggested in #63

@phillbaker
Copy link
Collaborator

Thanks for the context @lampietti, just so I get up to speed, a few more questions:

  • Does this break backward compatibility for those using gradle 3?
  • Since Google Play now requires apps to use a minimum of api level 26, what do you think about using that as the default level? Does that have the resources needed?
  • Were most of these changes generated by a script or by hand? If so, just curious what the process was so that we can document it.

@jasonmerino
Copy link

@lampietti @phillbaker would it be possible to get this merged? I'm being held up on an Android release because of it. Thanks!

@phillbaker
Copy link
Collaborator

@jasonmerino can you help answer the questions I asked above? That would help move this PR forward.

Does this break backward compatibility for those using gradle 3?
Since Google Play now requires apps to use a minimum of api level 26, what do you think about using that as the default level? Does that have the resources needed?
Were most of these changes generated by a script or by hand? If so, just curious what the process was so that we can document it.

@jasonmerino
Copy link

I'm no expert when it comes to Gradle, but AFAIK this would be a breaking change for those using Gradle 3.

I would think API 26 would be a fine fallback.

The changes in the Gradle wrapper (gradlew) file would have been generated code changes, I believe.

Hope that helps. Like I said, I'm not an expert on this but this is how I understand it. 🤞🏼

@mikehardy
Copy link
Collaborator

I would agree this is breaking for people using older react-natives (based on older Gradle versions). react-native-fs has a good example of the messaging for this as the break occurs at RN0.57 https://github.com/itinance/react-native-fs/

It is sort of a shame that this change also includes the example updates - those are useful but separate, really.

API27 is the default for RN0.58
API28 is the default for RN0.59 just released

As an android developer I don't see a difference between 26 and 27 really. 28 adds the requirement that you can't load the development bundle over HTTP without an exception for non-HTTPS, so it can be irritating. Apps should be controlling this directly though so the fallback is never really used.

All the gradle wrapper stuff is generated with ./gradlew wrapper yes

@phillbaker
Copy link
Collaborator

phillbaker commented Mar 13, 2019 via email

@phillbaker
Copy link
Collaborator

Just commenting that backwards compatible support for gradle 4 was introduced in #78. The plan, especially with react-native version 60 introducing required androidx support is to introduce those breaking changes at once.

@mikehardy
Copy link
Collaborator

not sure this is relevant anymore? (just scanning through these as I consider integrating this library again after a delay)

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.

4 participants