-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[iOS] Add CocoaPods Podfile to the project template #23563
Conversation
@fson same like in another PR. There should be iOS platform version 9.0 |
8048dc7
to
bb91638
Compare
@fson what's the status of this PR? |
This idea is a great first step. Thanks for working on making the experience of using React Native more convenient. I have a little question, have you tried running |
Some concerns regarding using CocoaPods by default. https://rnfirebase.io/docs/v5.x.x/installation/ios#Option-2:-Cocoapods-(Not-Recommended) But if we start recommending cocoapods we might be able to turn it around. |
If we do provide Should we include |
@zhigang1992 is that still valid (yes, I saw that note on rnfirebase too)? As far as I can see, proper support for Not an iOS/XCode/CocoaPods-expert myself, but came across this note too, and having support for CocoaPods seems a more "clean" approach IMHO. 👍 |
You should include them because it'd make builds reproducible. CocoaPods has generally been good at releasing new versions that are backward compatible but I'd better pin the version.
If the pods are part of the packages resolved by npm or yarn, I don't think there's a need to include this one @zhigang1992. I'd only include it if you have dependencies in your Podfile that are pulled from the central registry. |
I'm all for that, but when do we update this? What if I use a version later than the one used here, what then? |
@pvinis Gemfile is very similar to package.json you don't need to lock to specific version. But you can lock it to whole 1.X branch with |
I didn't get exactly what you mean. The Gemfile having cocoapods in it is not like package.json. It would be like if package.json had the yarn version in it. That's why I ask. Right now, in many RN projects I have a Gemfile already, with cocoapods and fastlane for example. I guess that would be alright if the template has some version and I modify it, same as the template having a flowconfig version for example, that I also have modified before. I guess there will be no problem, I'm just saying. |
@pvinis do you say about Gemfile or Podfile? If Podfile should be 100% compatible with very user, you need to specify minimal Cocoapods version. And for that you need Gemfile. |
I'll just wait for the example. I know what you are saying. I understand it. (I even prefer carthage for some reasons.) Anyway, forget what I said. :) |
@fson This PR will solve issue like this duplicated React framework invertase/react-native-firebase#414 (comment)? Because for now React comes as embedded and pod install adds another React pod |
@fson let me know if you'd need any help with this work. I'm happy to make a contribution to the project. |
@fson, what's the status here? We wanted to ship it with 0.60 and we are about to cut that release soon (in fact, we could do it as early as today). |
This PR should be ready to review and ship! (Sorry I had forgotten the |
pod 'React-jsi', :path => '../node_modules/react-native/ReactCommon/jsi' | ||
pod 'React-jsiexecutor', :path => '../node_modules/react-native/ReactCommon/jsiexecutor' | ||
pod 'React-jsinspector', :path => '../node_modules/react-native/ReactCommon/jsinspector' | ||
pod 'yoga', :path => '../node_modules/react-native/ReactCommon/yoga' |
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.
Not a blocker, but all of the above could be grabbed via a glob for Podspecs:
Dir.glob('../node_modules/react-native/**/*.podspec').each do |react_pod|
name = File.basename(react_pod, "podspec")
path = File.dirname(react_pod)
pod name, :path => path
end
This'll probably get deprecated by react-native-community/cli#256 eventually though
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.
This is so cool we can handle built-in podspecs with that API too.
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.
@orta this is cool and much nicer for version upgrades. However, the code above doesn't produce the same result, because not all podspecs in the code base are included by default. These libraries were not linked in by default in the old Xcode project and correspondingly those pods are not dependencies of the React
podspec:
./Libraries/ART/React-ART.podspec
./Libraries/CameraRoll/React-RCTCameraRoll.podspec
./Libraries/PushNotificationIOS/React-RCTPushNotification.podspec
./React/React-RCTFabric.podspec
./ReactCommon/fabric/graphics/React-graphics.podspec
./ReactCommon/React-Fabric.podspec
./ReactCommon/turbomodule/core/React-turbomodule-core.podspec
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.
Great point, I wonder if we make a way keep track of the default Podspecs inside React Native's codebase?
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.
Right now it's these podspecs:
Lines 45 to 57 in bbd98d5
s.dependency "React-Core", version | |
s.dependency "React-DevSupport", version | |
s.dependency "React-RCTActionSheet", version | |
s.dependency "React-RCTAnimation", version | |
s.dependency "React-RCTBlob", version | |
s.dependency "React-RCTGeolocation", version | |
s.dependency "React-RCTImage", version | |
s.dependency "React-RCTLinking", version | |
s.dependency "React-RCTNetwork", version | |
s.dependency "React-RCTSettings", version | |
s.dependency "React-RCTText", version | |
s.dependency "React-RCTVibration", version | |
s.dependency "React-RCTWebSocket", version |
The podspec structure is supposed to mirror the Xcode projects as closely as possible, so I put podspecs next to the corresponding Xcode projects, but perhaps we could have a designated folder for all "core" podspecs that should be picked up by default?
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.
Interesting thanks, I wonder if we can use that then - it's a great source of truth.
Could have the podfile read the React.podspec, and add those dependencies (might end up being a bit of code (because the file paths are non-deterministic) )
I'll have a think
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.
Thanks so much for working so hard on this. Let's ship it!
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.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @fson in cd8064b. When will my fix make it into a release? | Upcoming Releases |
Great work! I would recommend checking that archive works correctly as well, at least before release. Historically this has been somewhat of a pain point, where everything seemed to work fine, except archive would be broken, or the release build would crash. |
@msand could you add this check and send a PR? |
I actually think we should bring this back. I think it's a good thing to generate "a final" project we can just open and run. Right now, there's no |
Also, That leaves
|
Installing cocoapods for the first time is medium-fearsome. I just did it recently, I remember going "oh ___, I guess I google cocoapods then...okay should I use brew? or gem?...okay brew, then...oh wow that's a lot of data downloading (I have a small connection)...etc etc". You can tell which way I would vote based on dev-experience anyway - the template should come out of the box ready to start essentially, IMHO I'm actually installing it on a machine right now and it's going to take 30+minutes just for me to do the "git sync" part of pod update |
I think I would like an adaptive approach that does this:
That way creating a new project won't be slower or get in the way for people who are just getting started, and it comes with all the benefits when somebody is already familiar with these tools (assuming that installing the tools means some familiarity). What do you think? |
I can understand the reasoning against having CocoaPod as an additional build-dependency, so like @grabbou wrote, there should be a way to opt-in to using that @grabbou I would not favor for having a But to be honest, having nothing, like only the pure XCode-crap ... sorry, thats not useful, there has to be some way, like adding CocoaPod. Just add some lines to the "getting started" document and let "the community" give feedback to this. But I would see this as an optional step, not a forced one. |
Unfortunately, it will be slower, because installing CocoaPods and generating When users would open the project directory right after To fix that, we would have to run |
@FibreFoX that seems like an easy fix to not run git if you are already within a git repo. |
@cpojer yeah, with my proposal, we generate all files upfront so you don't need to have run it while init. The template will have React Native preinstalled already with files generated. First dependency linked is React Native itself, that's where's the "problem" we are talking about coming from. |
I'm on board. |
Maybe this can be added as some parameter instead of automagic? Another thing to keep in mind: not all users want to use CocoaPod, some are using Carthage. So leaving the parts inside the existing |
+1 on running I think for edge cases like Carthage support people will make their own templates, you already have to have a reasonable understanding of Xcode to use it in a react native project so I wouldn't call them the target users |
hm, it does suck though a bit though. maybe we can add the |
I'm curious about @onmyway133's comment above RE ending up with duplicate copies of React and its core dependencies. Ran into this when I tried switching from Carthage to CocoaPods. Other I guess the solution might be removing the Pods-managed version of React as outlined by @skurfuerst here. But if CocoaPods becomes the defacto way to manage libs via |
@fson I know this issue is closed, but curious about how to officially get CocoaPods working with RN. It's given us nothing but trouble, see my comment above. I'm concerned about commands like We're currently experimenting with https://github.com/orta/cocoapods-fix-react-native/ to deal with the pathing collisions/issues. But that shim is stale and seems to require updates to work with latest RN versions. |
@benjarwar having CocoaPods to be the recommended and default way of consuming React Native iOS files, we will automatically get rid of issues with collisions and Pod definitions becoming stale. If you have further concerns, please open them in the React Native CLI repository, where we can chat about anything that relates to |
@@ -474,6 +476,44 @@ | |||
shellScript = "diff \"${PODS_PODFILE_DIR_PATH}/Podfile.lock\" \"${PODS_ROOT}/Manifest.lock\" > /dev/null\nif [ $? != 0 ] ; then\n # print error to STDERR\n echo \"error: The sandbox is not in sync with the Podfile.lock. Run 'pod install' or update your CocoaPods installation.\" >&2\n exit 1\nfi\n# This output is used by Xcode 'outputs' to avoid re-running this script phase.\necho \"SUCCESS\" > \"${SCRIPT_OUTPUT_FILE_0}\"\n"; | |||
showEnvVarsInLog = 0; | |||
}; | |||
FD10A7F022414F080027D42C /* Start Packager */ = { |
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.
I'm curious, why exactly was this build phase added? I'm on RN v0.59.10, and I'm moving everything over to CocoaPods, and it seems like one side effect is that Metro doesn't start automatically anymore, and it looks like this build phase would fix that. Maybe this issue was also encountered when this PR was in progress; if so, does anyone know why exactly Metro doesn't start automatically without this?
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.
Mm, looks like the culprit was react-native-community/cli#317. That issue was closed too hastily, I think; I've posted a few comments there.
Summary
CocoaPods makes it easier to add new iOS dependencies to a project without having to manually edit Xcode projects. Editing Xcode projects is time consuming and merging changes to them difficult. Automating the changes to Xcode project
react-native link
is error prone. CocoaPods is a de-facto standard way to manage iOS dependencies and a central part of unimodules and upcoming improvements toreact-native link
.This PR adds a
Podfile
to the default project template of React Native. To use a project with CocoaPods, after creating it, runcd ios; pod install
and use the created<projectname>.xcworkspace
file instead of the.xcodeproj
file. (We could make this a part ofreact-native init
so you only need to run one command when creating a project.)Changelog
[iOS] [Added] - Add CocoaPods Podfile to the project template
Test Plan
Ran
yarn pack
in the repository root and then created a project and ran it successfully: