-
Notifications
You must be signed in to change notification settings - Fork 918
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
WIP: build: accept option for react-native project path #749
Conversation
From what I can see, iOS does not call If it does, where exactly does it call it? |
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. |
368493c
to
5631840
Compare
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 😊 |
7076820
to
71c7eec
Compare
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 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.
71c7eec
to
4cccdbd
Compare
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. I also wonder how other tools approach this problem. |
I think what needs to be solved is: • In which path to run 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.
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 🤷♂ |
I am wondering if we shouldn't leverage the
In that case, the place you're running If we would let it look up the tree, running it from This is aligned with What do you think? |
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) |
Yeah, that sounds good. |
So, if that is the preferred approach, please feel free to close this PR. |
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! |
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? |
Are you on React Native Community Discord? There's #cli channel there. If not, I can issue you an invite. |
Please send an invite 😊 |
Actually my bad, we already look for the configuration file recursively. Then I guess I need to better understand the root cause. Sending |
@tbergq, I have sent another PR #769 to remove the recursive reading of the configuration. |
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