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.

Sign up for GitHub

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

Jump to bottom

Gradle updates in android.js #24

Merged
merged 3 commits into from
Jul 16, 2019
Merged

Gradle updates in android.js #24

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f38443c
Update android.js
SaeedZhiany Jul 10, 2019
793a2a2
Fix embedding of braces in templates/android.js
Jul 10, 2019
f1d66ae
Update Gradle comment for gradlePluginVersion from RN 0.60
Jul 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions templates/android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,24 @@
module.exports = platform => [{
name: () => `${platform}/build.gradle`,
content: ({ packageIdentifier }) => `buildscript {
ext.safeExtGet = {prop, fallback ->
rootProject.ext.has(prop) ? rootProject.ext.get(prop) : fallback
}
repositories {
google()
jcenter()
}

dependencies {
// Matches recent template from React Native (0.59)
// https://github.com/facebook/react-native/blob/0.59-stable/template/android/build.gradle#L16
classpath 'com.android.tools.build:gradle:3.3.2'
// Matches recent template from React Native (0.60)
// https://github.com/facebook/react-native/blob/0.60-stable/template/android/build.gradle#L16
classpath("com.android.tools.build:gradle:$\{safeExtGet('gradlePluginVersion', '3.4.1')\}")
SaeedZhiany marked this conversation as resolved.
Show resolved Hide resolved
}
}

apply plugin: 'com.android.library'
apply plugin: 'maven'

def safeExtGet(prop, fallback) {
rootProject.ext.has(prop) ? rootProject.ext.get(prop) : fallback
}

// Matches values in recent template from React Native (0.59)
SaeedZhiany marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/facebook/react-native/blob/0.59-stable/template/android/build.gradle#L5-L9
SaeedZhiany marked this conversation as resolved.
Show resolved Hide resolved
def DEFAULT_COMPILE_SDK_VERSION = 28
Expand Down Expand Up @@ -53,7 +52,7 @@ repositories {
}

dependencies {
compile 'com.facebook.react:react-native:+'
implementation "com.facebook.react:react-native:$\{safeExtGet('reactnativeVersion', '+')\}"
Copy link
Contributor

@friederbluemle friederbluemle Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@SaeedZhiany - I'm curious to understand in which case this would be used? The version of react-native is directly declared by package.json and resolved by the package manager. When should it be overridden by "reactnativeVersion"? Can you provide a concrete example please?

Copy link
Contributor Author

@SaeedZhiany SaeedZhiany Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @friederbluemle, yeah you're right. I just think having more control over all version used in the sub-modules could be great. currently, also if you open your project in android studio (open the android folder), you can see that android studio shows warning about nondeterministic package version usage in app's gradle file in implementation "com.facebook.react:react-native:+". by this change, the warning could be solved.

However, I understand your point and agree with it, we can just ignore the warning and let the package manager solve the RN version. if you and @brodybits believe it has no more real usage than I mentioned, we can just revert it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation and for your work! :)
You're right that the "avoid using + in version numbers" warning will no longer show up, since the more complex expression can't be picked up by the linter anymore. It doesn't mean it's "solved". However, in this specific case, the warning can safely be ignored as the version is not dynamic. It is set in package.json (and most likely locked) which (as you know) is not directly visible to the Gradle build system.
I'm definitely with you in making builds more reliable and predictable. We just have to be careful to back any modifications to the default templates with strong use case and examples 👍
That said I have seen the reactnativeVersion mechanism in a handful of other projects, and just wasn't sure exactly how it can be beneficial to set/override the version like that. That's why I asked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you,

since the more complex expression can't be picked up by the linter anymore. It doesn't mean it's "solved".

Exactly and that's the reason I agree to revert it to +. When I changed this line, I wasn't very familiar with automatic version resolving of npm or yarn mechanism. (honestly, I hate warnings as much as errors :D so I just wanted to get rid the error)

I have seen the reactnativeVersion mechanism in a handful of other projects,

in which projects? can you give a link, please?

And finally, what do you think? should we revert it? I'm ready to submit a PR again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also please join this conversation and help us to find a way to manage androidX packages' version in the main project. the result of the conversation would be helpful for this repository too.

}

def configureReactNativePom(def pom) {
Expand Down