Skip to content

WIP: build: accept option for react-native project path #749

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

Conversation

tbergquist-godaddy
Copy link

Summary:

In monorepos, the root path, where node_modules live
might not be the same as where the actual react-native project is.

Test Plan:

I created a new react-native project, ran yarn android, and it still works. I tested it from our monorepo (sorry, it is closed source) and it works. I can provide you with open-source monorepo to test it later if necessary.

I still consider this to be work in progress. I want to do similar changes also for iOS.
Also, I would like to use commander for reading the rn-project-path, or if there is a better way, please suggest.

As for the commander, I still don't understand it fully, since when I try to read the command args before the loadConfig call, the program exits.

Also, maybe add some tests if it makes sense

closes #717

@tbergquist-godaddy
Copy link
Author

From what I can see, iOS does not call react-native config does it?

If it does, where exactly does it call it?

@thymikee
Copy link
Member

iOS uses config as well: https://github.com/react-native-community/cli/blob/master/packages/platform-ios/native_modules.rb#L14

Thanks for sending the PR :). I'll try to process it next week. We'd like to make a better story for monorepos and custom directory structures (there's a lot of open issues and PRs trying to address that, but we need to think it through). I'm not yet sure if it's gonna be by adding an extra flag, we'll see.

@tbergquist-godaddy
Copy link
Author

iOS uses config as well

Ah, yes it does 💡, but for my project setup, this does not need to change, since it is already called where my react-native-project is, and that works.

Which is probably why I thought I was not called at all 😊

@tbergquist-godaddy tbergquist-godaddy force-pushed the monorepo-setup branch 2 times, most recently from 7076820 to 71c7eec Compare September 30, 2019 06:14
@tbergquist-godaddy
Copy link
Author

Ok, I am beginning to understand that this problem is a little bit more complex.

The changes that I did so far allows me to build the android app, which without these changes crashes. However, images from 3rd party libraries are missing, like react-navigation-stack back button image.

On iOS it was building correctly and all assets like the back button is correctly in the development version. But they are missing in production build version 🤔

In monorepos, the root path, where node_modules live
might not be the same as where the actual react-native project is.
@grabbou
Copy link
Member

grabbou commented Oct 1, 2019

Hm, I have a feeling we have seen (or actually had) previous attempts on supporting monorepos in this project. What was the outcome of these?

I was thinking that any additional configuration like that (e.g. node_modules location) could be part of the configuration if that makes any sense.

I also wonder how other tools approach this problem.

@tbergquist-godaddy
Copy link
Author

I think what needs to be solved is:

• In which path to run react-native config (this PR)
• How to make sure bundle assets are added correctly, both for dev & production.

I don't understand why 3rd party assets are correctly bundled in on iOS for dev, but not for android. What are the platform differences there?

Also, I don't understand why these 3rd party differences disappears on iOS production build.

I was thinking that any additional configuration like that (e.g. node_modules location) could be part of the configuration if that makes any sense.

Maybe something like this. The metro bundler already has watchFolders, which makes it possible to tell metro which other folders to bundle code from. Maybe something like this is needed for the cli as well 🤷‍♂

@grabbou
Copy link
Member

grabbou commented Oct 3, 2019

In which path to run react-native config (this PR)

I am wondering if we shouldn't leverage the cosmiconfig ability to look for the configuration up the tree.

Cosmiconfig continues to search up the directory tree, checking each of these places in each directory, until it finds some acceptable configuration (or hits the home directory).

In that case, the place you're running react-native config from wouldn't essentially matter. Right now, we have specified the stopDir property, making it not look outside of the cwd.

If we would let it look up the tree, running it from packages/mobile would read it in packages/mobile, packages and . (and possibly even further up the tree, but that's probably acceptable trade-off).

This is aligned with require.resolve AFAIK and shouldn't cause problems when a command is executed at a different level than node_modules.

What do you think?

@grabbou
Copy link
Member

grabbou commented Oct 3, 2019

The big question here is: why we haven't turned it on by default?

I remember trying it out and hitting some issues. I guess it would be a good starting point to try to do it and see what happens (and observe our test suite blow up)

@tbergquist-godaddy
Copy link
Author

Yeah, that sounds good.

@tbergquist-godaddy
Copy link
Author

So, if that is the preferred approach, please feel free to close this PR.

@grabbou
Copy link
Member

grabbou commented Oct 3, 2019

All righty! Do you plan on working on that approach? I'd love to help (e.g. via Discord) if you have capacity. If not, don't worry!

@grabbou grabbou closed this Oct 3, 2019
@tbergquist-godaddy
Copy link
Author

I will try to have a look at it tomorrow (not promising anything), and I will let you know.

Is there a channel for this on discord, do you have a link?

@grabbou
Copy link
Member

grabbou commented Oct 3, 2019

Are you on React Native Community Discord? There's #cli channel there. If not, I can issue you an invite.

@tbergquist-godaddy
Copy link
Author

Please send an invite 😊

@grabbou
Copy link
Member

grabbou commented Oct 3, 2019

Actually my bad, we already look for the configuration file recursively. Then I guess I need to better understand the root cause.

Sending

@grabbou
Copy link
Member

grabbou commented Oct 3, 2019

@tbergq, I have sent another PR #769 to remove the recursive reading of the configuration.

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.

Properly configuring cli
3 participants