Skip to content

fix: do not read configuration outside of the root #769

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

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

grabbou
Copy link
Member

@grabbou grabbou commented Oct 3, 2019

Summary:

Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.

Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.

Context:

Fixes #655).

As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.

I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).

Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.

@grabbou
Copy link
Member Author

grabbou commented Oct 3, 2019

I believe that the test failures are unrelated.

@tbergquist-godaddy
Copy link

I believe that the test failures are unrelated.

On #749 this test also failed randomly.

@tbergquist-godaddy
Copy link

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.
This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂

If you want to see my setup, I have a monorepo here.

Still, the bundler cannot find the react-navigation back image.

@grabbou
Copy link
Member Author

grabbou commented Oct 4, 2019

@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.

@tbergquist-godaddy
Copy link

Ok, thank you @grabbou 🙂 I will follow both PR's closely

@grabbou
Copy link
Member Author

grabbou commented Oct 8, 2019

Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.

This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.

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.

Let's do it

@thymikee thymikee changed the title Do not read configuration outside of the root fix: do not read configuration outside of the root Oct 8, 2019
@thymikee thymikee merged commit 71f109e into master Oct 8, 2019
@thymikee thymikee deleted the fix/config-paths branch October 8, 2019 20:12
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.

Cannot get property 'packageName' on null object
3 participants