Gradle updates in android.js #24

SaeedZhiany · 2019-07-10T11:11:20Z

By these changes, developer's library can read android plugin version and react-native version of root project

templates/android.js

SaeedZhiany · 2019-07-11T17:12:05Z

@brodybits
can you help me in this PR to convince react-native framework developers that this changes are really needed and it should turn a convention among RN library developers?

brodycj · 2019-07-11T17:33:33Z

can you help me in this PR (facebook/react-native#25569) […]

I just took a quick look, will probably need some more time before I am ready to contribute to that discussion.

Even if that kind of a change is accepted, I think native modules cannot depend on it for a while if they need to support older React Native versions (which I think we should do by default).

brodycj

A few more comments:

As you can see from my suggested changes, I think we should update some of the comments if we update for React Native 0.60.

I would still favor updating the Android Gradle plugin to 3.4.1 in a separate PR, to be landed in a separate commit. I generally like to change one thing at a time. But I would be willing to reconsider my position, if the update works back to React Native 0.57. (I will do the testing.)

While we are making these changes, I think it would be nice if we could define the Android SDK, Android Tools, and Gradle plugin values all in one place. I tried to do this in April, as you can see in PR #31 which is not working for me. Any help on this would be appreciated.

I would finally like to thank you for the nice contributions so far.

brodycj · 2019-07-11T17:37:15Z

templates/android.js

@@ -9,17 +12,13 @@ module.exports = platform => [{
    dependencies {
        // Matches recent template from React Native (0.59)


Suggested change

// Matches recent template from React Native (0.59)

// Matches template from recent React Native version

brodycj · 2019-07-11T17:37:58Z

templates/android.js

@@ -9,17 +12,13 @@ module.exports = platform => [{
    dependencies {
        // Matches recent template from React Native (0.59)
        // https://github.com/facebook/react-native/blob/0.59-stable/template/android/build.gradle#L16


Suggested change

// https://github.com/facebook/react-native/blob/0.59-stable/template/android/build.gradle#L16

// https://github.com/facebook/react-native/blob/0.60-stable/template/android/build.gradle#L16

(please double-check that my change to the link is correct)

templates/android.js

SaeedZhiany · 2019-07-11T20:08:35Z

Yeah sure, I'll glad to be useful for RN world :)

I think native modules cannot depend on it for a while if they need to support older React Native versions

I agree with you but my vision is for long term as I said in my PR in RN repository,
to avoid building a project with multiple Android Gradle tools. RN world has continusly changing over time and library developers may haven't much time to keep their library up to date actively, even some of them don't take the time for accepting new PRs in their library and it's really headache for me to deal such inconsistencies. I just want to reduce such problems in RN projects development.

While we are making these changes, I think it would be nice if we could define the Android SDK, Android Tools, and Gradle plugin values all in one place. I tried to do this in April, as you can see in PR #31 which is not working for me. Any help on this would be appreciated.

Honestly, I didn't much familiar with how Gradle build system works so far and I try many ways to finally getting the changes work as I expect. recently I'm starting to read more about Gradle build system. maybe we can find a way to get it works as you suggested.
I will getting touch with you if I can find a way or any clue.

brodycj

Hi @SaeedZhiany, I changed my mind again. Your new gradlePluginVersion 3.4.1 update works for me on RN 0.57, 0.58, 0.59, and 0.60. I just pushed tiny changes to the comment block and approved this PR to be merged soon.

I would like to take care of a couple other things before merging this one. I will likely do a squash merge once I am ready.

Let’s disregard my other review for now. I would like to take care of the other comments someday in the future. Thanks again for the nice work!

SaeedZhiany · 2019-07-17T03:48:13Z

@brodybits Thanks, I'm happy it comes handy,
And I'll be glad if you can trace and help me to solve the inconsistency among all RN libraries one time for ever.
@dulmandakh, One of RN collaborator has a different opinion for removing such inconsistencies as he said here but I couldn't understand it. also, he was submitted a PR for his approach in react-native-keychain library. Maybe his idea is better than mine.

Please join us to find the best solution to this problem.

brodycj · 2019-07-17T04:18:16Z

[…] Please join us to find the best solution to this problem.

Hi @SaeedZhiany please put that whole request into a new issue so that I can track it more easily. I do think solving that problem would be mutually beneficial to all projects discussed here, including this one.

P.S. Gradle kts files are quite new for me so I cannot promise much of any wisdom related to these files.

SaeedZhiany · 2019-08-16T06:48:26Z

Hi, @brodybits,
I'm still trying to convince RN community to have more controllable android buildings. after releasing RN 0.60.0 and androidX support, this requirement goes beyond from gradle plugin version. So I've started a discussion here and I think you can help a lot there. can you join us?

friederbluemle · 2019-08-28T00:26:10Z

templates/android.js

@@ -53,7 +52,7 @@ repositories {
 }

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


@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?

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.

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.

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.

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.

Update android.js

f38443c

By these changes, developer's library can read android plugin version and react-native version of root project

This was referenced Jul 10, 2019

Updated links in generated build.gradle SaeedZhiany/react-native-create-library#1

Closed

Updates for React Native 0.60 #25

Closed

Fix embedding of braces in templates/android.js

793a2a2

brodycj mentioned this pull request Jul 11, 2019

[WIP] 0.6.0 update DRAFT WIP 1 #30

Closed

7 tasks

SaeedZhiany commented Jul 11, 2019

View reviewed changes

templates/android.js Show resolved Hide resolved

brodycj mentioned this pull request Jul 11, 2019

[WIP] Move default values in build.gradle for Android DRAFT WIP 1 - NOT WORKING #31

Closed

brodycj reviewed Jul 11, 2019

View reviewed changes

Update Gradle comment for gradlePluginVersion from RN 0.60

f1d66ae

brodycj approved these changes Jul 11, 2019

View reviewed changes

brodycj mentioned this pull request Jul 16, 2019

0.6.0 updates part 1 #32

Merged

brodycj changed the title ~~Update android.js~~ Gradle updates in android.js Jul 16, 2019

brodycj merged commit 2cfa520 into brodycj:dev Jul 16, 2019

friederbluemle reviewed Aug 28, 2019

View reviewed changes

brodycj mentioned this pull request Sep 13, 2019

[...] use the same Android Gradle build tools as the top-level project #121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gradle updates in android.js #24

Gradle updates in android.js #24

SaeedZhiany commented Jul 10, 2019

SaeedZhiany commented Jul 11, 2019

brodycj commented Jul 11, 2019

brodycj left a comment

brodycj Jul 11, 2019

brodycj Jul 11, 2019

SaeedZhiany commented Jul 11, 2019 •

edited

Loading

brodycj left a comment

SaeedZhiany commented Jul 17, 2019

brodycj commented Jul 17, 2019 •

edited

Loading

SaeedZhiany commented Aug 16, 2019

friederbluemle Aug 28, 2019 •

edited

Loading

SaeedZhiany Aug 28, 2019 •

edited

Loading

friederbluemle Aug 28, 2019

SaeedZhiany Aug 28, 2019

SaeedZhiany Aug 28, 2019

		@@ -9,17 +12,13 @@ module.exports = platform => [{
		dependencies {
		// Matches recent template from React Native (0.59)

	// Matches recent template from React Native (0.59)
	// Matches template from recent React Native version

	// https://github.com/facebook/react-native/blob/0.59-stable/template/android/build.gradle#L16
	// https://github.com/facebook/react-native/blob/0.60-stable/template/android/build.gradle#L16

Gradle updates in android.js #24

Gradle updates in android.js #24

Conversation

SaeedZhiany commented Jul 10, 2019

SaeedZhiany commented Jul 11, 2019

brodycj commented Jul 11, 2019

brodycj left a comment

Choose a reason for hiding this comment

brodycj Jul 11, 2019

Choose a reason for hiding this comment

brodycj Jul 11, 2019

Choose a reason for hiding this comment

SaeedZhiany commented Jul 11, 2019 • edited Loading

brodycj left a comment

Choose a reason for hiding this comment

SaeedZhiany commented Jul 17, 2019

brodycj commented Jul 17, 2019 • edited Loading

SaeedZhiany commented Aug 16, 2019

friederbluemle Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

SaeedZhiany Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

friederbluemle Aug 28, 2019

Choose a reason for hiding this comment

SaeedZhiany Aug 28, 2019

Choose a reason for hiding this comment

SaeedZhiany Aug 28, 2019

Choose a reason for hiding this comment

SaeedZhiany commented Jul 11, 2019 •

edited

Loading

brodycj commented Jul 17, 2019 •

edited

Loading

friederbluemle Aug 28, 2019 •

edited

Loading

SaeedZhiany Aug 28, 2019 •

edited

Loading