Use CocoaPods-compatible react-native-simple-toast.

[chrisbobbe]

As discussed in private chat with Greg and Ray and in the commit message, it's crucial for our native-code third-party dependencies to support CocoaPods. Thankfully, @remobile/react-native-toast was the only one of ours that didn't, and it's nice to see that it's been three years since it was last updated, as a probable explanation for that; we haven't yet seen a well-maintained dependency that still neglects to support CocoaPods. It's really easy to add a .podspec file, and there's an untouched open PR for that, so we could vendor @remobile/react-native-toast if it came to that.

There are some visual differences here, so please see these screenshots:

Currently, with @remobile/react-native-toast, toasts look like this:

With the change back to react-native-simple-toast, they would look like this:

I actually prefer the more rounded corners of the new one, and the use of transparency instead of a shadow effect, for iOS. EDIT: Incidentally, I just saw a very similarly styled toast in the Gmail app on iOS; if anyone needs more convincing, I'll try to reproduce it and make a screenshot (it involves some effort; I need to receive an email on an existing thread while viewing the thread).

[chrisbobbe]

It would be great if someone could please test that this still works on Android; it claims to just use ToastAndroid for Android, so there shouldn't be any problems, but if there are, we can easily accomplish that in our own code. But I'm currently struggling to get the project to run on Android (I suspect there's some residual state from one time I was looking into the AndroidX upgrade).

[gnprice]

But I'm currently struggling to get the project to run on Android (I suspect there's some residual state from one time I was looking into the AndroidX upgrade).

Mmm, yeah -- you should be able to clear that by doing rm -rf node_modules/ && yarn install. (Which, fortunately, is reasonably quick.)

This is basically a bug in the RN-world jetifier tool, which is a consequence of its fundamental design (namely that it rewrites source files in place, in node_modules/.) The effect is that if you're on a branch which has the AndroidX upgrade, and then switch back to a branch that doesn't, any libraries you have in node_modules/ which were edited for the upgrade stay edited.

(But if that doesn't clear it, or for any further investigation -- I'd be interested to debug in chat!)

[gnprice]

Running from this PR branch, I get the following error:

FAILURE: Build failed with an exception.

* What went wrong:
Could not determine the dependencies of task ':app:compileDebugJavaWithJavac'.
> Could not resolve all task dependencies for configuration ':app:debugCompileClasspath'.
   > Could not resolve project :@remobile_react-native-toast.
     Required by:
         project :app
      > Unable to find a matching configuration of project :@remobile_react-native-toast:
          - None of the consumable configurations have attributes.

I think the reason is that you need to edit the Gradle build config 😉

[chrisbobbe]

Mm. I've run rm -rf node_modules && yarn install many times throughout the CocoaPods work. It was an issue with a "sync" phase, which I don't really know anything about. But that's gone away for some reason, I'm not sure why. I ran it again, maybe 97th time's the charm? 😆

Ahh, and I just now saw some errors, like the one you quoted, that referred to the no-longer-used @remobile_react-native-toast! So, after applying this,

$ git diff
diff --git android/app/build.gradle android/app/build.gradle
index c0e876edb..53a5317ce 100644
--- android/app/build.gradle
+++ android/app/build.gradle
@@ -198,7 +198,6 @@ dependencies {
     implementation project(':react-native-text-input-reset')
     implementation project(':react-native-image-picker')
     implementation project(':react-native-orientation')
-    implementation project(':@remobile_react-native-toast')
     implementation project(':react-native-photo-view')
     implementation project(':rn-fetch-blob')
     implementation project(':react-native-sound')
diff --git android/settings.gradle android/settings.gradle
index 2d780e85e..c8fe94dd7 100644
--- android/settings.gradle
+++ android/settings.gradle
@@ -15,8 +15,6 @@ include ':react-native-image-picker'
 project(':react-native-image-picker').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-image-picker/android')
 include ':react-native-orientation'
 project(':react-native-orientation').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-orientation/android')
-include ':@remobile_react-native-toast'
-project(':@remobile_react-native-toast').projectDir = new File(rootProject.projectDir, '../node_modules/@remobile/react-native-toast/android')
 include ':react-native-photo-view'
 project(':react-native-photo-view').projectDir = new File(rootProject.projectDir, '../node_modules/react-native-photo-view/android')
 include ':rn-fetch-blob'

the current problem is this:

It says I can run a compile command with some flags to get more details. I'd really like to do that, but I pressed a green triangle button to get it to build, and I don't know the corresponding exact command to run. Do you know how I can follow their instructions?

[gnprice]

It says I can run a compile command with some flags to get more details. I'd really like to do that, but I pressed a green triangle button to get it to build, and I don't know the corresponding exact command to run. Do you know how I can follow their instructions?

You can run the Android build (and also the Android unit tests) with tools/test android. Running that on the updated PR branch now, I get this:

$ time tools/test android
Running android...
:react-native-webview:reactNativeAndroidRoot /home/greg/z/mobile/node_modules/react-native/android
/home/greg/z/mobile/android/app/src/main/java/com/zulipmobile/MainApplication.java:19: error: package com.remobile.toast does not exist
import com.remobile.toast.RCTToastPackage;
                         ^
/home/greg/z/mobile/android/app/src/main/java/com/zulipmobile/MainApplication.java:52: error: cannot find symbol
                    new RCTToastPackage(),
                        ^
  symbol: class RCTToastPackage
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
2 errors

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:compileDebugJavaWithJavac'.
> Compilation failed; see the compiler error output for details.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
[...]

I think those two are the errors, and the message in your screenshot is a warning which is unfortunately just noise.

So I guess one observation is that the Android Studio GUI is not as clear about what's an error and what's a warning as the CLI can be :-)

[gnprice]

For a direct answer to your question: the command line involves gradlew, and I looked up what tools/test is doing there:

run_android() {
    # ...

    (
        cd android

        ./gradlew -q :app:assembleDebug :app:assembleDebugUnitTest || exit

        # The `-q` suppresses noise from warnings about obsolete build config
        # in our dependencies from the React Native ecosystem.
        # But it also suppresses the names of tests that failed.
        # So on failure, rerun without it.
        ./gradlew -q :app:testDebugUnitTest \
            || ./gradlew :app:testDebugUnitTest || exit
    )
}

So { cd android && ./gradlew -q :app:assembleDebug; } would be a short version for running just the build. Or leave out -q, if investigating those warnings.

[gnprice]

Separately, for this sort of debugging question I recommend asking in chat in e.g. #mobile-team . I'll usually see messages there sooner than in a GitHub thread 🙂 . Plus other contributors are more likely to see it there and potentially benefit from the answer, or may be able to answer.

[chrisbobbe]

@gnprice said:

(But if that doesn't clear it, or for any further investigation -- I'd be interested to debug in chat!)

Oops, I'm sorry! My eyes totally skimmed past this first message with that recommendation. I'll be sure to move to CZO if I need any more help. Thanks for your help with this.

[gnprice]

No problem 🙂 Glad this is making fast progress!

[chrisbobbe]

OK, and I've removed those unnecessary imports in MainApplication.java! At first I thought it was weird that all I was doing on the Android side was pulling stuff out, without adding anything back in, like I had to do on iOS. But it's actually reassuring: it means there's no doubt that react-native-simple-toast is using RN's ToastAndroid, which is definitely simpler than there being some custom Android code that this library provides, even if it's a thin wrapper around what RN provides in ToastAndroid.

I tested on Android and it works now! In fact, I finally got set up with a real Android device (the office Galaxy S9)! (In the emulator, it took so long to do the initial /register that I wonder if there was some network misconfiguration; if so, I don't know what that might be, as the data has always eventually arrived.)

[rk-for-zulip]

I'm not sure this PR should be separated into two commits, incidentally. What happens in the intermediate state if the iOS app tries to show a toast?

OK, and I've removed those unnecessary imports in MainApplication.java! At first I thought it was weird that all I was doing on the Android side was pulling stuff out, without adding anything back in, like I had to do on iOS.

I hope you haven't been doing all this removal and adding-in by hand! That should all [0] be handled for you by

$ react-native unlink @remobile/react-native-toast
$ react-native link react-native-simple-toast

See (e.g.) the installation instructions for the latter.

[0] (Well, almost all. You'll still have to pull out a line for the react-native-toast code in MainApplication.java, because the change in indentation in that file means that react-native unlink doesn't recognize one of the lines it needs to remove. That results in a compile error rather than a link error, though, so it might have been quicker to diagnose.)

[chrisbobbe]

Thanks for the review, @ray-kraesig!

I'm not sure this PR should be separated into two commits, incidentally. What happens in the intermediate state if the iOS app tries to show a toast?

I can squash them, if you like, but nothing should go wrong on the first commit because the call site isn't switched over at that point; it'll still show the old @remobile/react-native-toast toast. The second commit does that, and removes @remobile/react-native-toast. I considered marking the first commit [nfc], but there's been at least one commit (577ac4c, I think) where code is added, but not yet used, where @gnprice suggested not marking it [nfc] because it's a crucial part of a decidedly non-[nfc] change, and that was my strategy here.

RE: react-native link and react-native unlink, these have seemed a little like magic, and they have some sometimes unwanted side-effects that I haven't proven would all be version-controlled (though the one I just linked to is), so I've been wary of them, especially after figuring out how to accomplish exactly what was required on iOS by editing the Podfile and the Xcode config (first directly in the Xcode config, then I found out how to do the same thing through the Xcode UI which was faster). I did give react-native unlink a try, to remove those lines on the Android side, but after I had unlinked manually on iOS first, which might be a weird, unhandled state, and might be why I got a weird error:

$ react-native unlink @remobile/react-native-toast
error Failed to get dependency config. Run CLI with --verbose flag for more details.

and it seemed easier to grep for all appearances in the code and trim them out manually (clearly I didn't get them all in one pass though).

[rk-for-zulip]

I can squash them, if you like, but nothing should go wrong on the first commit because the call site isn't switched over at that point; it'll still show the old @remobile/react-native-toast toast.

Ah! Sorry about that; somehow I saw the second commit as being the first commit. All's well here, then. 👍

they have some sometimes unwanted side-effects that I haven't proved would all be version-controlled (though the one I just linked to is), so I've been wary of them,

If the installation instructions for a library silently change things that aren't version-controlled, I think I would have some concerns about using that library at all. 👀

(The particular issue you've indirectly linked to is surprising – I don't think I ran into it in any of my 0.60 experiments. I have some hypotheses... but no way to test them, and they're not relevant to this PR anyway.)

At any rate, neither of these two libraries has a react-native.config.js, so their link and unlink shouldn't be doing anything beyond the default (which is just the tedious but standard plumbing). I'd suggest checking a diff between your final version and a version based on unlink + link – plus Xcode autoclean-on-save, probably – and making sure that you understand, and are okay with, any differences. (Optionally using git worktree or the like, if you're really concerned about edits to node_modules et al.)

[gnprice]

I can squash them, if you like, but nothing should go wrong on the first commit because the call site isn't switched over at that point; it'll still show the old @remobile/react-native-toast toast. The second commit does that, and removes @remobile/react-native-toast. I considered marking the first commit [nfc], but there's been at least one commit (577ac4c, I think) where code is added, but not yet used, where @gnprice suggested not marking it [nfc] because it's a crucial part of a decidedly non-[nfc] change, and that was my strategy here.

I think this strategy is quite reasonable. It'd be good to make it explicit -- in particular, when reading the first commit, to make clear that this just adds the dependency and makes it available but doesn't switch over to using it at all, and the next commit will do that. Otherwise it's natural to wonder, as Ray did 🙂

E.g. this could be a short paragraph right after the description of the changes (i.e. after the "So, run yarn add ..." graf).

I think it'd also work well in this case to squash them. One nice thing about squashing is that it'd make it more direct for the reader to match up removed and added bits, which mostly parallel each other and the ways they don't are interesting (like the fact that the old one had some Android stuff and the new one doesn't.) When one or both changes are more complex, the benefit to splitting them into separate commits grows, but these are relatively small.

[gnprice]

Nit in commit message:

It's not very clear why the switch was made, in f1356550b, but the
description of #M742 suggests that toasts weren't showing up on iOS;

Should be just #742 🙂 .

Other than that, and the one thing about foreshadowing the second commit more explicitly in the first one, LGTM!

[chrisbobbe]

If the installation instructions for a library silently change things that aren't version-controlled, I think I would have some concerns about using that library at all. 👀

This is unfortunately something we'll have to contend with, from RN core with RN v0.60.x onwards; the lowercase, RN-world "jetifier" tool will run automatically and modify things in node_modules in-place: see Greg's comment ("This is basically a bug in the RN-world jetifier tool, which is a consequence of its fundamental design (namely that it rewrites source files in place, in node_modules/.)") and doc ("Note that jetifier is included and ran automatically with react-native-community/cli for React Native versions 0.60 and above[...]"). So, helpfully or unhelpfully, the possibility was on my mind at the time.

I'd suggest checking a diff between your final version and a version based on unlink + link – plus Xcode autoclean-on-save, probably – and making sure that you understand, and are okay with, any differences.

That sounds right; I'll do this the next time I'm adding/removing dependencies. Or are you saying I should also investigate the react-native unlink failure this time, and go through that exercise now?

[rk-for-zulip]

If the installation instructions for a library silently change things that aren't version-controlled, I think I would have some concerns about using that library at all. eyes

This is unfortunately something we'll have to contend with, from RN core with RN v0.60.x onwards; the lowercase, RN-world "jetifier" tool [...]

Yeah. (jetifier is mentioned in the RN documentation and blog, though, which is the opposite of what I meant by "silently".)

That sounds right; I'll do this the next time I'm adding/removing dependencies. Or are you saying I should also debug the react-native unlink failure this time, and go through that exercise now?

I was saying that, but:

• the only debugging I'd thought would be necessary for the react-native unlink failure would be confirming that it doesn't happen from a clean slate. If that's not the case, I'd be concerned, but I think it would be a separate issue from this PR.
• honestly, it'll probably be fine without the additional verification.

[chrisbobbe]

Yeah. (jetifier is mentioned in the RN documentation and blog, though, which is the opposite of what I meant by "silently".)

Mmm, that makes sense.

honestly, it'll probably be fine without the additional verification.

OK, sounds good.

I've made those small changes Greg requested (let me know if you've asked for any that I've skimmed over by mistake). Now, Greg has helped me through a lot of other stuff with unimodules on czo (here and earlier), so I'll hopefully have #3987 ready for review not too long after this is merged. 🙂

[gnprice]

Looks good, thanks! Small comment below on the new bit.

[gnprice]

Ah, well spotted :)

I think it is slightly more helpful if this has the shape of a real example (even though which project it is doesn't really matter.) Saves any need to worry, as the reader, about whether things like < and > are part of the real error message or are meta-syntax.

For fun, here's a prediction of which project name would appear here instead:

I think the reason this error message came up this way is just that this was the first of our Gradle subprojects to get evaluated -- presumably because, at the time, it was the lexicographically-first of them, because it was the only one with a name starting with @. This entry was added in 4555fb2, and:

$ git show 4555fb22b:android/settings.gradle | grep ^include | LC_ALL=C sort
include ':@remobile/react-native-toast'
include ':app'
include ':react-native-device-info'
include ':react-native-fetch-blob'
[...]

(The LC_ALL=C environment-variable setting causes sort to sort by byte values, rather than the human-text-oriented sorting it does by default.)

So, today:

$ <android/settings.gradle grep ^include | LC_ALL=C sort | head -1
include ':@react-native-community_async-storage'

I think it would be project ':@react-native-community_async-storage'.

[chrisbobbe]

Interesting! Thanks for that investigation; I'll use that one then.

[gnprice]

And merged! Thanks again.