Skip to content
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: Do not require pod install after react-native init #24638

Closed
wants to merge 7 commits into from

Conversation

grabbou
Copy link
Contributor

@grabbou grabbou commented Apr 28, 2019

Summary

This pull request pre-generates CocoaPods files by running pod install locally inside a template before we publish it.

Rationale:

We have recently migrated default template from a regular iOS project to the one powered by CocoaPods. This was done to simplify set up required and make it easier to handle native modules in the future too.

Unfortunately, one drawback was that pod install was now going to be required to be run as an additional command, after react-native init has completed.

The detailed discussion and rationale can be found here: #23563 (comment)

Implementation:

We install Node dependencies inside a template. That also includes installing React Native, which at this point, points towards a previously generated tgz archive. Thanks to that, we are consuming an archive that will be published to the registry - same set of files.

Once dependencies are installed, we install CocoaPods dependencies (by running pod install command inside ios). Thanks to Node dependencies being installed, ../node_modules/react-native paths are resolved correctly.

Once Pods folder and Pods.xcodeproj has been generated, we remove node_modules and proceed to the next step, which is running react-native init itself.

At this point, we have Pods folder with all files generated. No steps required after react-native init completed - just react-native run-ios and we are set.

Note: Once CLI supports passing path to a template as .tgz instead of a folder, we should npm pack the template too. That way, we can make sure that Pods is present in the archive (and on npm).

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner labels Apr 28, 2019
@grabbou
Copy link
Contributor Author

grabbou commented Apr 28, 2019

Big thanks to @orta for pointing me towards a solution - I would loose my mind if it wasn't for you <3

@grabbou grabbou changed the title feat: Do not require pod install after react-native init [WIP] feat: Do not require pod install after react-native init Apr 28, 2019
@grabbou grabbou changed the title [WIP] feat: Do not require pod install after react-native init feat: Do not require pod install after react-native init Apr 28, 2019
@grabbou grabbou closed this Apr 28, 2019
@grabbou grabbou reopened this Apr 28, 2019
@grabbou
Copy link
Contributor Author

grabbou commented Apr 29, 2019

Now that react-native-community/cli#362 is going to be merged, I'll update the PR to always init from a tarball.

That way, we will be able to make sure that we test contents of a real package, not the source. This is important, because we have an explicit list of files to publish - we recently added Pods and I want to make sure we actually ship it.

@thymikee
Copy link
Contributor

FYI putting Pods into the tarball increases its size by ~12MB (from ~3MB to ~15MB). Not cool, as it affects everybody, not only those who init new project. We'll need to move the template to a separate package in the monorepo.

cd template

npm install
(cd ios && pod install)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll want to pass --repo-update flag so we publish up-to-date pods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... mind elaborating a bit what that flag is doing? We have local pods (from local spec within the repo, so no need to update the whole CocoaPods repo) and we also rm -rf Pods anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot it's local, you're right

@grabbou
Copy link
Contributor Author

grabbou commented Apr 30, 2019

Unfortunately, Pods cannot be npm publish'ed because Pods are using symlinks inside Pods/Headers and this will not be included in the package published (ref: npm/npm#6703 (comment))

There's no way right now other than requiring to run pod install after init that I am aware of.

Our tooling doesn't support this at all (e.g. we don't prompt to run pod install, we don't run it automatically) - we would need time to implement that properly. I feel like doing it right now, just in time for the 0.60 release, would be asking for potential troubles and degraded first-time experience.

I think it's important to make sure that initing a new app is as smooth as possible and we should pay an extra time to a) think about the best scenario, b) how to implement it and c) test it.

Possible next-steps:

  • hold with 0.60-release for another two weeks and do proper CocoaPods support in tooling for init experience (e.g. run-ios warn about missing Pods, suggest installing, prepare section in docs where we can explain this)
  • revert CocoaPods PR from 0.60-release
  • make default template a non-CocoaPods and add another one react-native-cocoapods to be enabled behind a flag (those who have CocoaPods can use it instead)
  • we are okay with disadvantages and want to ship anyway

@mikehardy
Copy link
Contributor

As a very recent new user that commented on the initial RFC related to "podless install" I can say that Pods is absolutely, without a doubt the way to handle iOS things. The problem is just unfamiliarity and expectations (for me). If the tools even spit out a big message that said "Before running react-native run-ios you must run pod install in the ios directory http://docsurl/react-native-and-pods.html" and on that page there was a very quick intro to pods (either brew or gems to install, big first time download, always open .xcworkspace) This would be fine. In other words, two quick bits of documentation to start getting devs up to speed. They are not that complicated, it's just new knowledge - but useful enough that the community would/should rally around it as a big new feature I think

@orta
Copy link
Contributor

orta commented Apr 30, 2019

It might be possible to duplicate the files for the headers instead of having the symlinks?

@grabbou
Copy link
Contributor Author

grabbou commented Apr 30, 2019 via email

@orta
Copy link
Contributor

orta commented Apr 30, 2019

My gut says no, CP (quite reasonably) probably didn't want to have to make duplicate files in the fs. I just tested it locally in one of my working CP projects, and replaced the links with the originals and it compiled file. Could be enough?

@grabbou
Copy link
Contributor Author

grabbou commented Apr 30, 2019 via email

@grabbou
Copy link
Contributor Author

grabbou commented Apr 30, 2019

Looks like this:

# Convert all symlinks inside Pods to real files
for f in $(find Pods -type l);
  do cp --remove-destination $(readlink $f) $f;
done;

will do!

Updated and testing in 0.60-stable branch. If it works, we are at home! I guess pod install down the road will convert them to symlinks anyway.

@grabbou
Copy link
Contributor Author

grabbou commented Apr 30, 2019

Unfortunately, converting symlinks to real files doesn't work as it produces "duplicated declaration" linker errors.

The only way to solve this would be to actually "restore symlinks" in init itself, but we would need to back in support to the CLI for that.

CC: @thymikee

@matt-oakes
Copy link
Contributor

I get the reasoning behind wanting to avoid users needing to manually run pod install after an init, however, I don't think trying to "fake it" is the way to go.

As the template uses Cocoapods now, users are going to have to use it at some point when they add any library with external dependencies. At that point they are going to need Cocoapods to be installed. Isn't it best to get them to install it along with the other required dependencies like Watchman and Node? All that is required is a call to sudo gem install cocoapods and they will have everything they need.

At that point, if we know (and can check) that they have Cocoapods installed, and we can just run pod install for them in the init command. It also means they are all setup and ready to go for when they add new dependencies.

# Set the React Native version to point to the `.tgz` file
node scripts/set-rn-template-version.js "file:$PACKAGE_LOCATION"

# We need to generate CocoaPods project. To do so, we install depdendencies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# We need to generate CocoaPods project. To do so, we install depdendencies
# We need to generate CocoaPods project. To do so, we install dependencies

@grabbou
Copy link
Contributor Author

grabbou commented May 6, 2019

I agree with you @matt-oakes. Unfortunately, there are too many hacks that need to be done in order to support this reliably.

I am going to consult it with the CLI team where and how we can add support for automatically running pod install and we will probably just notify that it's required to install if not present in order to run a React Native app.

@grabbou grabbou closed this May 6, 2019
@benjarwar
Copy link

@grabbou we've found integrating CocoaPods to be very challenging, to the point that we've historically opted to stick with Carthage for iOS dependency management. We're going to try switching to CocoaPods again, but I'm nervous about the potential disruption to our development process. Sorry to cross-post, but check my comment in #23563.

@thymikee
Copy link
Contributor

thymikee commented May 6, 2019

@benjarwar this is for new projects only. For users who would like to always start with Carthage, there's an option to create and release to npm a custom template (since RN 0.60.0 or @react-native-community/cli@next currently). It's very straight-forward to create one, see the docs: https://github.com/react-native-community/cli/blob/master/docs/init.md#creating-custom-template

Once you create it, one can do:

npx @react-native-community/cli@next init ProjectName --template sample-carthage-template

or when RN 0.60.0 RC is up:

npx react-native@^0.60.0-rc.0 init ProjectName --template sample-carthage-template

@benjarwar
Copy link

@thymikee thanks for the clarification. But there's mention in multiple comments in #23563 that react-native link is also going to be CocoaPods dependent.

@thymikee
Copy link
Contributor

thymikee commented May 6, 2019

We're moving away from link in favor of autolinking. See: react-native-community/cli@76f079e and https://github.com/react-native-community/cli/blob/master/docs/autolinking.md. I'm pretty sure something similar could be done for Carthage as well in userland :)

@matt-oakes
Copy link
Contributor

@grabbou What are the hacks which are needed and what is the issue with running it directly? It is because you're not sure if the user has Cocoapods installed through Bundler or a similar tool?

@grabbou
Copy link
Contributor Author

grabbou commented May 7, 2019 via email

@matt-oakes
Copy link
Contributor

@grabbou I think we're agreeing. I was suggesting that pod install is run on the user side while initing. The hacks you mention seem to be for running install before publishing the template, unless I'm getting confused.

@grabbou
Copy link
Contributor Author

grabbou commented May 7, 2019 via email

@hramos hramos deleted the feat/ship-cocoapods branch February 25, 2020 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants