Use CocoaPods

[chrisbobbe]

EDIT: Adding unimodules was split out into #4016.

Fixes #3983. Also adds react-native-unimodules, to prepare for #3964.

Tested on the simulator and device, and all features seem to be working. Possibly a little slower at runtime than before, at least initially, maybe not significantly.

Note in the second commit that we're stuck on a version of react-native-unimodules (0.6.0, no caret) that's not the latest, because at some point between 0.6.0 and 0.7.0-rc.4 (reportedly), support for RN v0.59 was dropped. 0.7.0-rc.4 is reportedly the earliest version that supports AndroidX, so it may be that our AndroidX work (PR #3852) will have to be done simultaneously with the RN v0.60 upgrade (issue #3548). Apparently, backwards compatibility for those on older RN versions is not a priority; see https://github.com/unimodules/react-native-unimodules/issues/100#issuecomment-566436685.

[gnprice]

Great! Glad to see this so quickly. 😄

I'll have to go connect the Mac I have here (taken home from the office when we all started working from home) to a monitor, so I can play with this... and hmm, first clear some desk space for said monitor. 🙂

0.7.0-rc.4 is reportedly the earliest version that supports AndroidX

Hrm. As I wrote over on #3852, when an app updates to use AndroidX, it can do so even while it's still using third-party libraries that are still built to use the Android Support Library; so the migration path is, apps go first, then libraries update at their leisure.

There are a lot of people on the Internet confused about that, though -- I didn't really understand it until I did the research that produced #3852. It's possible there's something nasty react-native-unimodules is doing that actually interferes with that nice smooth migration path engineered by the AndroidX authors. But I suspect that instead the library's maintainers are just among the people confused, and that in fact it'll work just fine. I guess we can find out!

[chrisbobbe]

It's possible there's something nasty react-native-unimodules is doing that actually interferes with that nice smooth migration path engineered by the AndroidX authors.
[...]
But I suspect that instead the library's maintainers are just among the people confused, and that in fact it'll work just fine.

Mm! Good point. The deprioritization of backwards compatibility and semantic versioning made me leery to dig deeper into the code. It seemed possible that the best record for the current state of the package was in its maintainers' heads, so I tended to trust them when they said certain versions wouldn't work nicely with AndroidX. But you're right, and now I remember the results of your research into AndroidX: it should Just Work (unless, like you said, react-native-unimodules has done something unexpected).

[chrisbobbe]

I'll need to update the docs in that first commit, also, because pod install is a required command now. Just tested on a freshly cloned project; the build fails without running pod install (of course) but running pod install (after running yarn; order matters, since the Podfile uses paths in node_modules) is sufficient to get it up and running.

[gnprice]

Some small code comments, and one larger substantive one.

[gnprice]

nit: trailing whitespace (highlighted by git diff by default)

[gnprice]

Let's drop the references to ancient RN versions -- we are in fact on >= each of these 🙂

[chrisbobbe]

OK. It's very difficult to find the officially recommended example Podfile for RN v0.59 (see facebook/react-native-website#1603 (comment)), so including all of its details (with those outdated comments) was a goal, in case we encountered any issues with these dependencies in the future.

But I do link to that officially recommended example Podfile for RN v0.59, in a code comment at the top, which I guess is enough. And it's true that we're >= each of those versions. I would just hope that if, e.g., 'CxxBridge' starts causing an issue, we don't find some SO answer that says we can just remove it, and do that, seeing no justification for its existence.

[gnprice]

Yeah. There's that link; and also when asking that kind of "why did we put this here?" question, that's always a good prompt to go looking in the Git history, so that'll give us the full context of why we added these lines.

Also ideally when we've fully digested this migration, each subspec we mention here will be here because we really do need it, and have a clear reason why. 🙂 For a chunk of them the reason might be like "these are core pieces of RN used for everything", but which ancient version they were introduced in isn't really part of that information. In our current state of knowledge, basically that link is the explanation.

[gnprice]

nit: a 9-digit commit ID ded79d2cf makes this a bit more readable

[gnprice]

Hmm, does this belong yet at this commit?

[chrisbobbe]

Mm, I thought I fixed that (I certainly saw it, at some point), but obviously not.

Originally, I did everything at once — the CocoaPods work and adding unimodules — and made a single commit. Then, when I thought it would be good to split that into two commits, my strategy was to create a commit after the original commit, deleting the code that added unimodules. And then to reverse the commits.

But to accomplish that reversal, I knew that a simple interactive rebase with the two pick commands reversed wouldn't work. That's because the "pick" command applies a computed patch, it doesn't just effect the selected commit's snapshot. Usually applying a patch is what we want, but in this case, I had snapshot A and snapshot B, and I simply wanted to reorder them so B came before A.

I ended up doing an interactive rebase and breaking before the first commit, then doing git checkout <secondCommit> -- ., committing that, then git checkout <firstCommit> -- ., committing that, then git rebase --edit-todo, I dropped the two original commits, and finished the rebase. I'm not sure if this was exactly where this mistake was introduced, but it sure seemed like a lot of maneuvering just to reverse the two commits. Do you know of a better way?

[gnprice]

Usually applying a patch is what we want, but in this case, I had snapshot A and snapshot B, and I simply wanted to reorder them so B came before A.

One way I might do this is:

git reset --soft @^^
# Now your HEAD includes neither commit, but your worktree and index are still at B.

git commit -c @{1}
# Now you have one commit, with tree/snapshot B.

git checkout @{2}^ .
# Now the worktree and index are at A.

git commit -c @{2}^
# Now you have both commits.

The -c are optional, but convenient for preserving the author dates and commit messages (but still give you an editor to edit the commit messages.)

The @^^ and @{2}^ and so on could be replaced with explicit commit IDs copied from git log, but make handy ways to refer to the commits without needing to copy-paste.

When the commits in question are in the middle of a longer series, I might use git rebase -i with just a b / break command inserted after the two commits, and then do the above steps there.

[chrisbobbe]

Thanks, I've bookmarked this!

[gnprice]

Hmm, so, I think there's a next step to be done before we're really fully using CocoaPods. That is to name here all the subspecs we need... and remove all the corresponding pieces from our own Xcode project file. In this version, it looks like all these pieces of RN are still incorporated directly into our build there.

In fact, as long as they're mentioned directly in our Xcode config, I'll be a little uncertain whether we've really fully wired up CocoaPods at all, because it seems like things would keep working even if we haven't.

On the other hand, once we have done that -- and then gone on to do that for the various third-party dependencies we use too -- our project.pbxproj should be quite a lot simpler than it's ever been before! This Podfile looks like a much cleaner and more readable way of tracking that same information.

It looks like you can find all the available subspecs by looking in node_modules/react-native/React.podspec.

Ah, and then from searching through ios/ZulipMobile.xcodeproj/project.pbxproj for mentions of a couple of these, I think you can find something like a list of all the ones we're using with the command grep '/node_modules/react-native/.*\.xcodeproj' ios/ZulipMobile.xcodeproj/project.pbxproj. Or this variant:

$ perl -lne 'print $1 if (m{/node_modules/react-native/.*/(.*)\.xcodeproj})' \
    ios/ZulipMobile.xcodeproj/project.pbxproj
RCTActionSheet
RCTGeolocation
RCTImage
RCTNetwork
RCTVibration
RCTSettings
RCTWebSocket
React
RCTPushNotification
RCTLinking
RCTText
RCTAnimation
RCTCameraRoll
ART

[chrisbobbe]

That is to name here all the subspecs we need
[...]
It looks like you can find all the available subspecs by looking in node_modules/react-native/React.podspec.

In the Podfile, should I aim to reference all of the subspecs that are listed in React.podspec? I took the example Podfile from the RN v0.59 doc thinking that was the minimum set of subspecs necessary, and that anything on top of that might be pointless or introduce bugs. I can see the argument for being more complete than we may need to be, but I wonder why the example Podfile doesn't include all the subspecs in React.podspec, if that's the intended behavior for React Native dependencies.

Do you think "all the subspecs in React.podspec" includes just the outer level of nesting, here, or might we also need to get the "subspecs of the subspecs" (if that makes sense):

$ grep '\.subspec \"[a-zA-Z]*\" do' node_modules/react-native/React.podspec

  s.subspec "Core" do |ss|
  s.subspec "CxxBridge" do |ss|
  s.subspec "DevSupport" do |ss|
  s.subspec "RCTFabric" do |ss|
  s.subspec "tvOS" do |ss|
  s.subspec "jsinspector" do |ss|
  s.subspec "jsiexecutor" do |ss|
  s.subspec "jsi" do |ss|
  s.subspec "PrivateDatabase" do |ss|
  s.subspec "cxxreact" do |ss|
  s.subspec "fabric" do |ss|
    ss.subspec "activityindicator" do |sss|
    ss.subspec "attributedstring" do |sss|
    ss.subspec "core" do |sss|
    ss.subspec "debug" do |sss|
    ss.subspec "graphics" do |sss|
    ss.subspec "scrollview" do |sss|
    ss.subspec "text" do |sss|
    ss.subspec "textlayoutmanager" do |sss|
    ss.subspec "uimanager" do |sss|
    ss.subspec "view" do |sss|
  s.subspec "RCTFabricSample" do |ss|
  s.subspec "ART" do |ss|
  s.subspec "RCTActionSheet" do |ss|
  s.subspec "RCTAnimation" do |ss|
  s.subspec "RCTBlob" do |ss|
  s.subspec "RCTCameraRoll" do |ss|
  s.subspec "RCTGeolocation" do |ss|
  s.subspec "RCTImage" do |ss|
  s.subspec "RCTNetwork" do |ss|
  s.subspec "RCTPushNotification" do |ss|
  s.subspec "RCTSettings" do |ss|
  s.subspec "RCTText" do |ss|
  s.subspec "RCTVibration" do |ss|
  s.subspec "RCTWebSocket" do |ss|
  s.subspec "fishhook" do |ss|
  s.subspec "RCTLinkingIOS" do |ss|
  s.subspec "RCTTest" do |ss|

[rk-for-zulip]

Hmm, so, I think there's a next step to be done before we're really fully using CocoaPods. That is to name here all the subspecs we need... and remove all the corresponding pieces from our own Xcode project file.

That's certainly something we're going to have to do at some point, but it may be more complexity than is needed for #3964. Does react-native-unimodules need a full CocoaPods conversion, or will a bare-bones Podfile without the React Native core components suffice? (The latter would probably still be a significant step towards #3983.)

[gnprice]

Does react-native-unimodules need a full CocoaPods conversion, or will a bare-bones Podfile without the React Native core components suffice? (The latter would probably still be a significant step towards #3983.)

Yeah, if the fuller conversion turns out to be more complex, I'd be happy merging whatever smaller version suffices to unblock #3964.

Though again, I'll be more confident that we've successfully fully wired up CocoaPods when there are pieces of the build that we rely on and don't have some other way they get included 😉 Without that, it seems like we might attempt #3964 and then discover there's something else we need to do in order for a library we add there to actually get built into the app.

[gnprice]

In the Podfile, should I aim to reference all of the subspecs that are listed in React.podspec? I took the example Podfile from the RN v0.59 doc thinking that was the minimum set of subspecs necessary, and that anything on top of that might be pointless or introduce bugs. I can see the argument for being more complete than we may need to be, but I wonder why the example Podfile doesn't include all the subspecs in React.podspec

What I think I'd ideally like is to just specify the exact same set as we currently do in the Xcode project file, and at the same time stop specifying those there. I.e. just convert over how we include those pieces, separate from any changes to which ones we include.

It's also pretty likely that there are some things we include in the build that we don't need to, largely because the Xcode config has felt so opaque and it's felt so finicky, and liable to cause problems that are annoying to debug, to modify it. So after they're all in this Podfile instead, we might go through and identify some we don't need and remove those.

[chrisbobbe]

and remove all the corresponding pieces from our own Xcode project file

This sounds easier to solve than determining the correct contents of the Podfile. So far, I haven't found a clear way forward for how to cleanly remove things from the project.pbxproj file. I think this area of the Xcode UI will be helpful:

Note that libPods-ZulipMobile.a is in that list, and a lot of other, mostly RN-related stuff is also in the list. It seems plausible to want to shrink that list down to just the Pods item, adding each removed item to the Podfile. But for that I'll need a clear mapping from a given .a item in that list, to a corresponding line to add to the Podfile.

[chrisbobbe]

Mm, our posts crossed! 🙂Still, to accomplish

specify the exact same set as we currently do in the Xcode project file

it sounds like the next step is for me to find a mapping from all of those .a files in that screenshot (assuming that screenshot controls the Xcode project file in the ways we want), excluding the libPods-ZulipMobile.a file, to pods/subspecs that can be added to the Podfile. That counts on there being an available Pod, and it may count on some other things too, and I'll make it my next focus to find out. 🔍

[gnprice]

I'll need to update the docs in that first commit, also, because pod install is a required command now. Just tested on a freshly cloned project; the build fails without running pod install (of course) but running pod install (after running yarn; order matters, since the Podfile uses paths in node_modules) is sufficient to get it up and running.

Can we put that in postinstall in our package.json? See https://github.com/zulip/zulip-mobile/pull/3852/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2 for an example.

If there's a part that's hard to automate, then let's document; but when we can automate, automating is better 😉

Hmm, I guess we probably need to add "install CocoaPods" to the dev setup instructions, too.

And then the postinstall should only run pod install if present / if you're on macOS. Probably that's too much logic to stuff into a package.json line, so we should just say "postinstall": "tools/postinstall" there and have a script at tools/postinstall that handles it. In that case, perhaps best to first merge a version where pod install is just in the docs, and follow up to automate it.

[chrisbobbe]

Yay! Thanks to Greg's help, we're well on our way to getting this working.

I'm investigating two other issues now (a postinstall script to avoid double-counting some font files for RN Vector icons, motivated by oblador/react-native-vector-icons#1074 (comment), and I've seen an instance where Metro doesn't start automatically, but there are clues for a workaround with an extra Build Phase in facebook/react-native@cd8064bc5), but one seems ripe for others to work on independently, so I thought I'd jot it down before lunch here.

One of our dependencies, @remobile/react-native-toast, lacks a .podspec file. It's very easy to add one (this article agrees, and gives a call to action for PRs to do that), but it hasn't been done yet. In fact, there's a three-year-old PR to do that: remobile/react-native-toast#20, but it hasn't been merged. It appears that there haven't been any updates to that repo at all, for three years. So it may be time to vendor it (make our own local fork with the desired changes), or to find another library that gives us toasts on iOS, that's well-maintained and either has a .podspec file or would readily accept a PR that adds one. (Toasts on Android are handily provided by ToastAndroid from React Native.)

Note that the .podspec file changes with an upgrade to RN v0.60.0, and it's easy for library maintainers to miss this, or introduce breaking changes by mistake; I'll dump a candidate commit message here with links to see an example:

    deps: Lock [email protected]; breaking change at 0.10.16.
    
    See the warning and apology for this in their README at
    https://github.com/joltup/rn-fetch-blob#version-compatibility-warning.
    
    Specifically, at
    https://github.com/joltup/rn-fetch-blob/commit/0f6c3e3cc (released
    in [email protected]), the Podspec was updated to match RN's new
    pattern (added in
    https://github.com/facebook/react-native/commit/2321b3fd7, released
    in [email protected]) of having separate podspecs for each Xcode
    project, instead of using subspecs for them.
    
    We can allow upgrading to 0.10.16 when we switch to
    [email protected] (that's open as #3548).

[chrisbobbe]

Adding "blocked on other work" for #4008.

[gnprice]

to avoid double-counting some font files for RN Vector icons, motivated by oblador/react-native-vector-icons#1074 (comment),

Looking at the comment above that one:

it seems like the crux of the problem is that the font files are being referred to by both the react-native.config.js and the podspec.

the reference to the font files as assets in the react-native.config.js will cause the react-native link to load the fonts as a resource for the App project.

points to a cleaner solution, I think: it's saying that because the font files appear in react-native.config.js, they've wound up in our Xcode project config for our app. And indeed they seem to be there:

$ git grep vector-icons ios/
ios/ZulipMobile.xcodeproj/project.pbxproj:              2E41C86FA25744F998C2BB02 /* Zocial.ttf */ = {isa = PBXFileReference; explicitFileType = undefined; fileEncoding = 9; includeInIndex = 0; lastKnownFileType = unknown; name = Zocial.ttf; path = "../node_modules/react-native-vector-icons/Fonts/Zocial.ttf"; sourceTree = "<group>"; };
ios/ZulipMobile.xcodeproj/project.pbxproj:              51E1EDA028654E17A35C5BCA /* MaterialIcons.ttf */ = {isa = PBXFileReference; explicitFileType = undefined; fileEncoding = 9; includeInIndex = 0; lastKnownFileType = unknown; name = MaterialIcons.ttf; path = "../node_modules/react-native-vector-icons/Fonts/MaterialIcons.ttf"; sourceTree = "<group>"; };
[...]

So if we delete those references and let the podspec take care of it, that should resolve the conflict.

[gnprice]

And it perhaps says something about the sprawling opaque complexity of doing anything with Xcode build config 😝 that people in that thread are finding it easier to run a sed script on a dependency's podspec file than to delete the items from their own app's Xcode config.

[chrisbobbe]

because the font files appear in react-native.config.js, they've wound up in our Xcode project config for our app

But node_modules/react-native-vector-icons/react-native.config.js is in node_modules, so a postinstall script editing that file is the most reliable way we have of making sure it doesn't contain those references, and that they never get re-introduced, right? I'm not fully sure how, but it sounds like we're both seeing that their presence there is why they wind up in our Xcode config. Removing them from the Xcode config is necessary, but will it be enough? If they've flown from node_modules to the Xcode config before, might they do so again?

sprawling opaque complexity of doing anything with Xcode build config 😝

Yup. Luckily, I recognized the sed script on the podspec idea pretty quickly as something we don't want to do; we want to use the podspec file as-is, while deleting the stuff from the Xcode config (we just have to make sure it stays deleted).

[chrisbobbe]

Separately, one person believes that using CocoaPods with React Native allows us to use the "Parallelize Build" option in Xcode. I haven't thought through this enough to say whether it's likely to be a good idea (whether we'll ever get build failures because some things shouldn't be built in parallel), but I'm still able to build without failures, and I don't notice any runtime errors either. However, one trial without parallelization took 26s, and one trial with parallelization took 24s, so it doesn't seem to shave off that much time anyway. But here's the article that gave me the idea, anyway; it's possible that its claims are valid, but I'm just not sure: https://engineering.brigad.co/demystifying-react-native-modules-linking-ae6c017a6b4a#4f80

[gnprice]

making sure it doesn't contain those references, and that they never get re-introduced, right? I'm not fully sure how, but it sounds like we're both seeing that their presence there is why they wind up in our Xcode config.

I think react-native link is the thing that looks at react-native.config.js, and acts on it by putting something into our Xcode config.

In any case, the Xcode config is in version control -- so once the references are out, we'll notice if they come back in, and can easily take them out again 🙂 (and debug why they were re-introduced)

[chrisbobbe]

I think react-native link is the thing that looks at react-native.config.js, and acts on it by putting something into our Xcode config.

That makes sense. react-native link can be called with no arguments, just like yarn install, so it's tempting to approach it similarly to yarn install, as in "Well, I've tried a bunch of specific things, why don't I run this all-purpose, idempotent command that's supposed to fix all kinds of issues with my dependencies?" Basically, we can't recommend that approach for react-native link. But I don't think we've ever explicitly recommended it, so there's not much lost there. And the question will be moot once we move from automatic linking, with react-native link, to autolinking, without it. (Gotta love those names. 😬)

In any case, the Xcode config is in version control -- so once the references are out, we'll notice if they come back in, and can easily take them out again 🙂 (and debug why they were re-introduced)

Yep, that's true. And modifying things in node_modules in-place adds its own headaches that we've had to think about, even very recently. So it'll be nice to avoid doing that; I'm on board with not making a postinstall script. 🙂

[gnprice]

Re parallelizing the build:

• Sure, we probably should!
• I'm not sure this is related to switching to CocoaPods, at least not directly. From what I can tell from the web -- and the closest thing I can find to actual documentation on this is a WWDC talk 😞 -- the unit of parallelization that this is about is the "target". In particular that means that... in that post's example, the box has no effect at all, because there's just one target shown there.
  • Within a target, there are generally tons of different subtasks, compiling individual files. I can't immediately tell whether Xcode already parallelizes those, or does after this is enabled. I suspect that it already did -- that it builds a whole target, with individual files in parallel where applicable, then the next target, etc. -- and that it continues doing so with this box checked. But that's just a guess.
  • It may be that most of our parallelization opportunities are within a large target, rather than between many small ones. In that case this setting may not make a big difference.
• One thing that will strongly affect how much this helps is how many cores your CPU has. On the office Mac (which I have with me for this WFH period), there's just one physical core, two virtual "hyperthreaded" cores. YMMV.

[chrisbobbe]

Makes sense. I've separated that into its own commit, of course; I'm so glad to have something that doesn't have to be included in the giant commit I'm putting together.

Just to jot down a note that I found some Expo packages that will be available to us with unimodules, which we can use instead of some of our third-party dependencies if we so choose. Expo packages will be much better maintained, especially compared with any non-Expo, non-RN-Community counterparts that are designed to do the same thing. There may be others, but here's a list so far:

• ScreenOrientation (it looks like this is meant for RN 0.60+; their podspec depends on "React-Core" instead of "React", and it's always been that way for this module: https://github.com/expo/expo/blob/master/packages/expo-screen-orientation/ios/EXScreenOrientation.podspec)
• SafeAreaView (actually I haven't confirmed that this works with unimodules)
• DocumentPicker
• ImagePicker (maybe also Image, ImageManipulator and ImageLoader?)
• MediaLibrary
• WebBrowser
• Device
• Font (I'm not sure if this would help us with Solution found: Newer Unicode emojis don't render on older Android devices #3885, probably not; I don't see a lot of detail on any use it makes of Android's native dynamic font loading system)
• Application
• Notifications

Basically I'm looking at https://github.com/expo/expo/tree/master/packages and going to see if there's a ios/*.podspec.

[chrisbobbe]

OK, I've pushed my work for today -- this includes the branch from #4008, but that's only because it's necessary to get working code; that PR should be reviewed and merged before this one.

[gnprice]

formatting nit: last line should end with newline too

[gnprice]

Ah, good to fix this too, thanks!

[gnprice]

Ah indeed -- the React.xcodeproj seems a strong indication that this can go away too :)

[gnprice]

Thanks again @chrisbobbe !

This looks great. One thing mentioned below which has a one-line fix, and there's a formatting nit above. I'll fix them both up and merge.

[gnprice]

Hmm, I suggested this above, but: I'd forgotten that in this script, the working directory is ios/! See the cd command above.

Probably this is a sign that I should revise this script to use the repo root as its working directory, same as our other scripts. 🙂 For the moment, I'll just fix this up while merging.

[chrisbobbe]

Ah, right, makes sense!

[gnprice]

Followup items collected from above (and please comment if you see one I've missed):

• small, good to do soon: Use CocoaPods #3987 (comment):

  it also sounds right, even if we do automate it, to simulate not having successfully run pod install, and add a troubleshooting entry for that; I can do that tomorrow.

• bigger, for another time: Use CocoaPods #3987 (comment)
  (move "Start Metro" script into a script file, make it not run on release build)

And then of course #4016 is the major next step 🙂 , taking us toward #3964.

[chrisbobbe]

Followup items collected from above (and please comment if you see one I've missed):

• The actual automation of pod install with a postinstall script (probably small)
• Small (hopefully?), not introduced in this PR: potential cleanup of "libz.tbd in Frameworks": Use CocoaPods #3987 (comment)

  This one isn't in the upstream template -- but it also looks like it never was. It entered our project file in a18c95b (Initial Implementation of Sentry. #881).

  Well, I guess that makes any cleanup of it orthogonal to this PR, then.

• This one just occurred to me — we have to note in the docs that people should open .xcworkspace instead of .xcodeproj in Xcode.

[chrisbobbe]

I responded about libz at #4026 (comment).