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 Android Gradle setup #463

Merged
merged 1 commit into from
Nov 13, 2019
Merged

Conversation

friederbluemle
Copy link
Contributor

@friederbluemle friederbluemle commented Nov 12, 2019

Trying to open or build the android/ folder was not possible, due to a missing buildscript block:

ERROR: Plugin with id 'com.android.library' not found.

After adding the reference to the source of the library plugin, the following error showed up:

ERROR: Failed to resolve: androidx.transition:transition:1.1.0

This was caused by missing entries in the repositories block. Added the necessary references to resolve the errors.

It is now possible to individually open and build the android/ folder in Android Studio (or build it on command line, after generating a Gradle wrapper).

Note that these changes will not affect consumer (app) projects - The Android Gradle plugin is included conditionally, meaning the dependency classpath only gets added when the project is opened standalone. This is better than simply removing it (as seen in #148)

@friederbluemle
Copy link
Contributor Author

@osdnk - Please take a look if you have a moment. Thanks!

@SaeedZhiany
Copy link

@friederbluemle

Thanks

I just took a look at the android project folder, there is no gradle wrapper folder, is it necessary to be added too?

@friederbluemle
Copy link
Contributor Author

A matching Gradle wrapper is generated automatically by Android Studio when you open the folder (you will notice Android Studio prompting you). Since the primary use case of a library is to be included in other projects, the Gradle wrapper should not be packaged with it (and also does not have to be in source control). The repo is already set up like this, so I did not add it here.

Alternatively, you can also generate a Gradle wrapper on command line by running:

gradle wrapper --gradle-version 5.4.1 --distribution-type all

@SaeedZhiany
Copy link

SaeedZhiany commented Nov 12, 2019

Thanks for your information, I have seen some repositories that have such a folder. Do you think it's a good idea to submit PR on that repositories and remove the folder from the repositories?

for example, you can see this repository and this one

@friederbluemle
Copy link
Contributor Author

Yes, many library project repos have the folder you mentioned in source control. For an application project, the Gradle wrapper should definitely be committed to source control - for a library project there is no "100% correct answer" - Some people like to have it, some don't. What's more important (and less controversial) is that all Gradle wrapper files need to be excluded from distribution. That is done using an appropriate combination of .npmignore/.gitignore and files declaration in package.json.
The setup react-native-reanimated chooses is not to have Gradle wrapper for the library folder in source control (which is also my preference).

@SaeedZhiany
Copy link

I got it, Thanks

@friederbluemle
Copy link
Contributor Author

Rebased onto latest master branch.

@osdnk
Copy link
Contributor

osdnk commented Nov 13, 2019

Hi, @friederbluemle
Why do you need this?

@friederbluemle
Copy link
Contributor Author

Hi @osdnk - The quick version is: To fix the errors you can see above. The longer version is: Without it, it's an incomplete project setup. Gradle/Android Studio does not know where to get the plugin from (when opened standalone - it will only work when opened together with an app project). Why would you want to open it standalone? Quite simply, it allows the project to be compiled and verified without any extra setup and files. It will be easily possible to write unit tests that only operate on the library code. You will also be able to lint the code in isolation (catching many errors that would otherwise go unnoticed).
Note that as I said above, this will have no effect on app consumer projects.

@osdnk
Copy link
Contributor

osdnk commented Nov 13, 2019

None of this cases is reasonable in Reanimated so far, but I’m fine with merging it if you really need

}
mavenCentral()
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not super familiar with these configs, but are you sure it won’t affect anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not affect anything. mavenCentral is the "old" entry which has been replaced by jcenter (which is a superset of mavenCentral).

@friederbluemle
Copy link
Contributor Author

friederbluemle commented Nov 13, 2019

I think being able to write standalone unit tests and being able to easily lint, compile and test the code in isolation is something that any repo would benefit from. I hope you agree, and since this won't have any negative effect on app projects, it would be great if you could merge. 🙏

@@ -20,17 +35,22 @@ android {
}

repositories {
mavenLocal()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

// Matches the RN Hello World template
// https://github.com/facebook/react-native/blob/1e8f3b11027fe0a7514b4fc97d0798d3c64bc895/local-cli/templates/HelloWorld/android/build.gradle#L21
url "$projectDir/../node_modules/react-native/android"
url "$rootDir/../node_modules/react-native/android"
Copy link
Contributor

Choose a reason for hiding this comment

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

And this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's taken directly from the main react-native repository. I did not make any additional changes to the repos listed here. It's also the way plenty of other react native library repos are set up. If you have a different opinion, please let me know and I'll change it.

@osdnk
Copy link
Contributor

osdnk commented Nov 13, 2019

Btw, you mentioned some tests and linting so I assume you use them, right? Do you want to include them as well?

@friederbluemle
Copy link
Contributor Author

Btw, you mentioned some tests and linting so I assume you use them, right? Do you want to include them as well?

Yes, good point. I do have some experimental setup (across many locally cloned RN library repos). It's not at a point for public use yet, but I'll keep this repo in mind for potential future integration. As a prerequisite, the changes in this PR are definitely required. Thanks!

@osdnk
Copy link
Contributor

osdnk commented Nov 13, 2019

So maybe let’s wait until you find these tests ready? :)

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