Skip to content

fix: Android gradle config when bundling for release #28415

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

Closed
vdmtrv wants to merge 4 commits into from
Closed

fix: Android gradle config when bundling for release #28415

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b458622
fix: Android gradle config when bundling for release
vdmtrv Mar 27, 2020
f7f93f7
fix: RNTester cliPath
vdmtrv Mar 27, 2020
977e9d7
revert: correct path to react-native cli
vdmtrv Mar 27, 2020
603c632
fix: typo in exception message
Mar 27, 2020
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
1 change: 1 addition & 0 deletions RNTester/android/app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ plugins {
*/

project.ext.react = [
cliPath: "../../../node_modules/react-native/cli.js",
bundleAssetName: "RNTesterApp.android.bundle",
entryFile: file("../../js/RNTesterApp.android.js"),
root: "$rootDir",
Expand Down
3 changes: 2 additions & 1 deletion react.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ afterEvaluate {
execCommand.addAll([*nodeExecutableAndArgs, cliPath])
}
} else {
execCommand.addAll([npx, "react-native"])
throw new Exception("Missing cliPath or nodeExecutableAndArgs from build config. " +
"Please set set project.ext.react.cliPath to the path of the react-native cli.js");
Copy link
Copy Markdown
Contributor

@hramos hramos Mar 27, 2020

Choose a reason for hiding this comment

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

Thank you for taking your time to improve this experience! Do you think we should mention android/app/build.gradle explicitly in the error message so it is clear where the config is located?

Copy link
Copy Markdown
Contributor Author

@vdmtrv vdmtrv Mar 27, 2020

Choose a reason for hiding this comment

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

You're correct. It's probably worth adding. I was simply following the convention on line 49:

throw new Exception("OS not recognized. Please set project.ext.react.hermesCommand " +
                        "to the path of a working Hermes compiler.");

If we include the path in this exception message then it needs to be added to the one on line 49.

Copy link
Copy Markdown
Contributor

@hramos hramos Mar 27, 2020

Choose a reason for hiding this comment

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

I'm OK merging this as is, since it follows convention. I'm not as familiar with Gradle, so my main worry is whether these error messages are clear to the average Android developer. Would they know where to go to fix this config? Does the config location vary across workspaces or is it generally expected at the same path?

In other words, if the config is always at android/app/build.gradle then perhaps we can explicitly point to that file.

}

def enableHermes = enableHermesForVariant(variant)
Expand Down