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

Using SDK Version variables from root project #248

Closed
wants to merge 4 commits into from

Conversation

rayronvictor
Copy link

Instead of assuming the compileSdkVersion, targetSdkVersion, etc, read it from the root project.
Default compileSdkVersion and targetSdkVersion to the latest versions.

Android Target API Level 26 will be required in August 2018.
https://android-developers.googleblog.com/2017/12/improving-app-security-and-performance.html
And the React Native team is already working on this:
facebook/react-native#17741
facebook/react-native#18095

Instead of assuming the compileSdkVersion, targetSdkVersion, etc, read it from the root project.
Default compileSdkVersion and targetSdkVersion to the latest versions.

Android Target API Level 26 will be required in August 2018.
https://android-developers.googleblog.com/2017/12/improving-app-security-and-performance.html
And the React Native team is already working on this:
facebook/react-native#17741
facebook/react-native#18095
Copy link

@taranda taranda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change did not work for me in React Native 0.56.

FAILURE: Build failed with an exception.

* Where:
Build file '/Users/dev3/workspace/ATC-Mobile/node_modules/react-native-keep-awake/android/build.gradle' line: 4

* What went wrong:
A problem occurred evaluating project ':react-native-keep-awake'.
> Could not find method google() for arguments [] on repository container of type org.gradle.api.internal.artifacts.dsl.DefaultRepositoryHandler.

I had to make these changes to get this to work with a new app in React Native 0.56.

These changes also mirrors React Native 0.56's build.gradle. I think the changes below will be more backward compatible.

@@ -1,22 +1,31 @@
buildscript {
repositories {
jcenter()
google()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend replacing google() with:

maven {
    url 'https://maven.google.com/'
    name 'Google'
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rayronvictor we should also swap google and jcenter due to recent issues with resolutions from jcenter

//noinspection GradleDynamicVersion
implementation "com.facebook.react:react-native:${_reactNativeVersion}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend replacing implementation with compile.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementation is the newest way to add dependencies. If you use compile, it throws a warning to replace it

Copy link

@taranda taranda Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may be true, but if you are using the default version on gradle that ships with React Native 0.56, the project will not run with implementation I've tested both, and only compile works with new React Native 0.56 projects.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry but implementation must be used as by the end of 2018 old configs like compile would be removed completely

Copy link

@taranda taranda Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend you create a new react native project and then install and link react-native-config. Running react-native run-android will fail to build if you use implementation. It probably has something to do with the version of gradle react native uses out of the box. I'm just trying to keep this project compatible with React Native's default settings.

I'm not an Android developer, so maybe there is something else in my local environment that is preventing implementation from working.

In any event, I recommend testing the final PR with a brand new React Native project to ensure it works.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @taranda I know what you are trying to refer, I have a bit of experience with these gradle like issues. so the reason it will fail on new created project is that "Gradle config coming from react-native 0.56.0 is not Gradle 3+" thus we see that error. That error most likely will fix by react-native 0.57.0 because react-native must have to 💯 support Gradle 3+ as per Android new requirement, "Compile is obsolete and would be removed by the end of 2018" along all SDK new changes etc. Thus, we can either update now or wait for react-native to release 1 more new version with Gradle 3.

Copy link

@taranda taranda Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. In that case, I'd recommend some adding something to the README. It seems React Native is slow to keep up with Android's requirements.

@DimiTech
Copy link

DimiTech commented Oct 17, 2018

Can we merge this in please and create a new release? @pedro @yeomann @taranda @ignaciosantise @rayronvictor

This is pretty urgent since it solves problems with RN >=0.56 when compiling apps using this package.

Thank you very much!

@taranda
Copy link

taranda commented Oct 17, 2018

@DusanDimitric, only @pedro has permission to merge PRs and I do not think he is maintaining this project any more. See Issue #269. I hope he is okay. I ended up forking this repo and making the changes myself.

@pedro
Copy link
Contributor

pedro commented Oct 17, 2018

hey folks! I'm so sorry for totally dropping the ball here, I'll definitely review this and other pending things and ship a new release this week 👌

@DimiTech
Copy link

Awesome man, thanks! This is an important package for a lot of people - 23,633 dls/week ATM :)

@lkknguyen
Copy link

@pedro thx Pedro, we're also share the same issue for our app and need your click of a button too :)

@okrand
Copy link

okrand commented Oct 22, 2018

@pedro any updates on this?

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.

10 participants