-
Notifications
You must be signed in to change notification settings - Fork 904
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
feat: support autolinking
in monorepos
#768
Conversation
I remember looking into And this would only work with one |
Speaking to @thymikee, we have decided to pursue using Will resume later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks too simple to be true :D so far I didn't find anything suspicious, but we need to test it thoroughly
We do not need root anymore. However, we still accept one argument (config) for testing purposes. Without this check, users that pass custom root would accidentially break their projects as CLI would take it as a config value.
autolinking
in monorepoautolinking
in monorepo
Added Android support. Feedback is welcome :) |
Great job :) |
@@ -57,10 +63,10 @@ def use_native_modules!(root = "..", config = nil) | |||
end | |||
|
|||
podspec_dir_path = Pathname.new(File.dirname(podspec_path)) | |||
project_root = Pathname.new(config_root) | |||
|
|||
relative_path = podspec_dir_path.relative_path_from project_root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we don't have custom root, I think instead of using project_root
we need to use cwd, because that's we look for – a relative path from cwd (ios dir) to our native module (in node_modules or custom path). This fixes the Slider repo example for me:
relative_path = podspec_dir_path.relative_path_from project_root | |
relative_path = podspec_dir_path.relative_path_from Dir.pwd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
projet_root
is project_root = Pathname.new(config["project"]["ios"]["sourceDir"])
few lines earlier - so that's exactly the location of ios
folder.
PWD might be not a good idea, see: #657 when running from root folder with --ios-directory
flag
It might be good to test this on the React Native Firebase monorepo test project also (https://github.com/invertase/react-native-firebase/tree/master/tests), though I have no availability until probably end of next week 😫 |
Will test React Native Slider and React Native Firebase before we ship it, just to be sure. |
React Native Slider isn't properly configured right now and this PR leveraged its issues. Running The following configuration makes everything run smoothly: const root = __dirname;
module.exports = {
project: {
android: {
sourceDir: './example/android',
},
ios: {
project: './example/ios/example.xcodeproj',
},
},
dependencies: {
'react-native-slider': {
root,
},
},
}; Note that in the future, once we add support to For example, no |
Note: I wouldn't recommend doing React Native Slider setup in the future. Ideally, every project goes React Native Firebase way, having a separate In that scenario, no custom configuration at all is needed. I still feel like custom |
Doesn't this configuration leak to the users if config is published? I think it's about time to end this hack (because it clearly was one, I forgot how bad :D). The now seemingly correct approach there would be to include a real package.json with |
@thymikee I actually just wrote this down and I am totally on board with your idea - I believe this is the best way to set it up and doesn't include this hack. |
autolinking
in monorepoautolinking
in monorepos
@thymikee we should have a generator for initing such a structure like React Native Firebase to work on. Once it's supported here by default, we can make it official and the only use-case supported. |
@grabbou sounds like something that Bob could provide |
React Native Firebase works w/o issues with this PR @Salakar. Great project set up by the way! |
Thanks for checking! |
@grabbou Hi. Could you help me? Should both dependencies be added to |
Yeah some examples of this would be nice, because Im still battling to no-hoist or not, to make sure that all is packaged right. Im following a couple of leads: https://engineering.brigad.co/react-native-monorepos-code-sharing-f6c08172b417 But keep getting missing in the project on some files and even getting past that I get firebase issues: invertase/react-native-firebase#2122 So as the commentes in the medium says, is it still too early to do react native monorepo? |
Fixes #537
Fixes #717
Fixes #657 (path to
.bin
no longer hardcoded)Fixes #633
The problem:
There are various issues complaining about weak or broken support for mono repositories, in regards to auto-linking. The workarounds are temporary and error-prone, being executed on top of a confusing resolution system.
Surprisingly, we already have quite a good support for
mono-repo
in the CLI. For example, we check for a package innode_modules
up the directory tree. We also do that for resolving the configuration file.In this PR, I am reworking some of the fundamental assumptions about the paths within auto-linking in order to make it support various project structures without any extra work.
Automatic mono repository support for automatic linking 😂
Summary:
Please look at the structure that I am working on. I believe this is the most common mono repo:
This structure is based on the @brunolemos repository that he provided for reproduction purposes.
Please keep in mind the following two design decisions:
Test:
When running
pod install
on iOS and Android, bothreact-native-webview
andreact-native-firebase
are linked properly with their relative paths.Status:
Let me know if this structure is similar to what you are running and what you'd expect.
Changes
root
in auto-linking and introduce automatic detection of the project locationPackageManager
to not rely on asetProjectRoot
being called in different placesprocess.cwd()
being set by few functionsdetachedFunction
(internally) that runs without configuration (useful for init and other commands that don't need project context)