Skip to content

Conversation

@grabbou
Copy link
Member

@grabbou grabbou commented Mar 18, 2020

TL;DR:

Fixes #995
Fixes #405
Fixes #793
Fixes #804

Improves #824 that was reverted via #852 because it caused regressions (see case 2)

Context:

React Native CLI is flexible and makes no assumptions where and how you structure your project folders (at least that's our ideal scenario). However, Gradle and CocoaPods need to call the React Native CLI to read the configuration, list of all dependencies to link and get some relative paths. In order to resolve where the React Native CLI currently is, Gradle and CocoaPods ask node and its require.resolve strategy to locate where react-native is located.

In 99% cases, Gradle is called from the project root directory or a folder within the project root, such as android (when you run ./android/gradlew). That makes require.resolve to work correctly, by locating the react-native in the closest node_modules traversing up the tree.

However, there's that 1% of use cases when this is not true. For example, Gradle can be sometimes given an absolute path to a project to build (via -p flag), which will result in a cwd that is different than the project root and outside of JavaScript hierarchy. This is very common for tools such as fastlane or app center and happens more often when doing a release of the apk rather than installing on a simulator.

Case 1

For example, calling: <path_to_gradle>/.gradlew -p Repositories/HelloWorld/android from /home/grabbou will result in the #995 error, because cwd is /home/grabbou and there's no /home/grabbou/node_modules/react-native to find.

Case 2

The previous jsAppDir workaround contributed by @thymikee wasn't optimal either, as it was inferring root of a project from the directory where node_modules are. In a monorepo, the dependency will be hoisted, which will result in the root being set incorrectly.

At the time of reverting this feature in #852, we didn't know why it was causing us troubles. However, recently, thanks to additional reproductions provided by the community, we were able to understand why that particular issue was happening.

For example, calling <path_to_gradle>/.gradlew -p Repositories/HelloWorld/packages/app-a/android with the aforementioned workaround would set root to /users/grabbou/Repositories/HelloWorld because React Native dependency has been hoisted to /users/grabbou/Repositories/HelloWorld/node_modules/. In reality, we want the actual /users/grabbou/Repositories/HelloWorld/packages/app-a.

Solution:

The solution turned out to be simple and as I initially expected, was a matter of finding the right Gradle variable to read from.

Reading rootProject.projectDir and using that as a root always points out correctly to the folder that is either inferred automatically or set via -p flag.

Test Plan:

Run gradle with -p flag outside of JavaScript hierarchy.

* Sometimes Gradle can be called outside of JavaScript hierarchy. Detect the directory
* where build files of an active project are located.
*/
def projectRoot = rootProject.projectDir
Copy link
Member Author

Choose a reason for hiding this comment

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

For context: rootProject is what you give a name in settings.gradle - it's the umbrella in case you have different smaller sub-projects

https://github.com/facebook/react-native/blob/master/template/android/settings.gradle#L1

For autolinking and CLI, it doesn't matter - we always want the main project. But you will notice that applyNativeModulesBuildGradle expects project argument to handle such case.

@grabbou
Copy link
Member Author

grabbou commented Mar 18, 2020

After this PR, I'll provide a similar workaround for iOS. Just in case somebody ends up calling pod with --project-directory.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Makes sense! I'm not sure why I couldn't find rootProject.projectDir variable

@grabbou
Copy link
Member Author

grabbou commented Mar 18, 2020

Added e2e tests to make sure this particular use casework. It's a good starting point for the future work around CLI and interactions on a "living" project.

I would say let's merge it and it if ends up being flaky, I am volunteering to fix it ASAP. Don't want to do premature optimisation tho.

grabbou and others added 2 commits March 18, 2020 15:41

// Execute `gradle` with `-p` flag and `cwd` outside of project hierarchy
const {stdout} = spawnScript(gradleWrapper, ['-p', androidProjectRoot], {
cwd: '/',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set cwd here? I'd expect a default value would be sufficient – this way we could make the options argument of spawnScript optional with {} as default value, as it used to be

Copy link
Member Author

Choose a reason for hiding this comment

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

Truth is - we never want to rely on default value in our tests and that's why I decided to go with explicit cwd.

In this case, I just want to make sure and be 100% explicit that cwd is somewhere else than the project - for example, at the root.

Copy link
Member Author

Choose a reason for hiding this comment

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

Truth is - we never want to rely on default value in our tests and that's why I decided to go with explicit cwd.

The reason why I believe it's better to require cwd is simple - cwd defaults to the location where the React Native CLI source files are located (or where the tests are being run from).

Conceptually, I think of e2e tests as something run in a totally separate environment that is close to what users are doing in real life. I feel like setting cwd in spawnScript (which I consider advanced use case, where runCLI is not enough) is just a way to tell "watch out, you're going into advanced territory - think about what you are doing".

@lylest

This comment has been minimized.

@thymikee thymikee deleted the fix/custom-root branch April 8, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants