Support ARTPushRegistererDelegate on iOS 14

[QuintinWillison]

Fixes #1069.

[QuintinWillison]

So far this PR just presents the groundwork. The next step is to offer an alternative route for actively injecting the delegate.

[QuintinWillison]

Good progress today but still a way to go. We're now building and running tests successfully in Xcode 12. Remaining to do, as far as I'm aware:

• Fix new warnings raised by Xcode 12's build
• Add and implement new interface for supplying delegate

[ricardopereira]

[lawrence-forooghian]

I say 🎉 to the fact that:

• the codebase can be built in Xcode 12 and the tests are passing there!
• we have a way for users of the SDK to avoid the issue described in Push Notification Registration via ARTPushRegistererDelegate don't work in iOS 14 when instantiated via @UIApplicationDelegateAdaptor #1069!

I’ve had to spend quite a bit of time looking at this PR to roughly understand what’s going on here. A large amount of this is definitely because I’m new to the company, the codebase, and the problem space. But I also think that we could spend a bit of time thinking about how this PR is presented, to see whether it could be made friendlier for reviewers, and whether there’s anything we might want to think about when we present our work for code review in the future. I found myself doing quite a lot of guesswork to understand the intent behind the various components of this PR.

I’ll try and explain my thought process. I realise that I’ve spent more time here commenting on the structure of the PR than engaging with its exact contents. And if I’ve completely misunderstood what's going on here and wasted a bunch of people's time, then I'm very sorry.

How have we fixed #1069?

The PR description says that it fixes issue #1069. So I was a little surprised to see that there was no further mention of what we’ve done to fix this particular issue - neither in the pull request description nor the commit messages.

Issue #1069 describes a symptom - a library user’s app is crashing when the app delegate is instantiated in a specific manner new to iOS 14. Before we can fix the symptom, I think we first need to understand the underlying cause. The user who raised the issue has offered their speculation as to the cause (“I believe UIApplicationDelegateAdaptor is downcasting MyAppDelegate to UIApplicationDelegate”) but I think our first action should be to confirm whether this is true, and if it isn’t then to identify the actual cause.

This probably requires us to begin by reproducing the issue ourselves. I did a little bit of investigation here, creating a test app to see how protocol conformance of the app delegate is affected by UIApplicationDelegateAdaptor. The reporter of the issue isn’t too far from the truth - when this property wrapper is used, SwiftUI creates an app delegate of its own special internal class SwiftUI.AppDelegate, which in some way wraps an instance of the MyAppDelegate class. The important fact is that it wraps it in a way which preserves the methods which the app delegate responds to, but does not preserve protocol conformance. Perhaps you knew all of this already, but this is definitely useful context and motivation for the changes of this PR, and I think it would be very valuable to include in a commit message.

Have we fixed #1069 in two different ways?

It seems to me like the very first commit of this pull request (cc03343) by itself would have fixed the issue reported in #1069, given the underlying cause explained above.

With that in mind, I’m a little confused about why we then build functionality that allows a push registerer delegate to be passed in to the SDK, instead of using the application’s sharedDelegate. I’m not saying that it isn’t a good idea - indeed, it seems like quite a good one from a “don’t mess with singletons you didn’t create” point of view. But it would be great if the commit messages could help (a reviewer / future new developers / future us) understand the motivation for this additional code - why is it required in order to fix #1069?

Why is this PR the right place to get the SDK working with Xcode 12?

So, it seems like this PR has three main themes:

1. Fix Push Notification Registration via ARTPushRegistererDelegate don't work in iOS 14 when instantiated via @UIApplicationDelegateAdaptor #1069 (possibly in a couple of different ways);
2. Get the codebase working with Xcode 12, for some value of “working” which I’m keen to understand better;
3. Drop support for iOS 8.

It’s not 100% clear to me why these three things are in the same PR. And I mean that in the following senses:

• Getting things working with Xcode 12 seems like a large enough and self-contained enough change that it deserves its own PR. That PR would be a great space to help reviewers understand what exactly it is that’s now working (see sub-heading below).
• Similarly, I’m guessing there’s some context (possibly a business decision) behind the decision to drop iOS 8 support. That context could be captured in a commit message and I think would sit well in its own PR.
• How does getting the app building in Xcode 12 help us to fix Push Notification Registration via ARTPushRegistererDelegate don't work in iOS 14 when instantiated via @UIApplicationDelegateAdaptor #1069? I’m guessing that it’s because we wanted to (manually?) test that we’ve fixed the issue by building an app with the iOS 14 SDK, using UIApplicationDelegateAdaptor, and confirming that the crash doesn’t happen. But I struggle to understand the connection by purely looking at the contents of the PR.

What exactly is now working, Xcode 12-wise?

As I said above, I get the sense from this PR that there is a theme of “make the SDK work on Xcode 12”. But what does that mean? And what doesn’t it mean? There are quite a few things that one might reasonably expect to be included in a “make it work on Xcode 12” set of changes. For example:

• It can now be built with Xcode 12, where it couldn't before?
• The tests are passing when running on Xcode 12, where they weren’t before?
• There is now some sort of support for iOS 14 that there wasn’t before?
• We’re now using Xcode 12 to build for CI?

I think that if there were a separate PR for the Xcode 12 changes, then that would give us a chance to explore which of these we have and haven’t done, and why. And hence tells reviewers which things have been accidentally missed and which things the PR submitter has deliberately left outstanding, so the reviewer knows where to focus their attention.

A few other comments on commit structure

I’m approaching the commit structure from a “how could this be more reviewable” point of view here.

• Definitely a subjective one, but I think that commits a78d0f3, 888e564, 26a5e43 could go into a single commit - “Push registerer delegate can be explicitly specified” or something like that. Particularly with 888e564 and 26a5e43 - new functionality and the tests for it are probably a single logical change.
• Commit b66cc4c8 is just an alteration to some changes introduced in 888e564, so I don’t think it warrants its own place in the commit history. Instead we could rebase and fixup the original commit. (If this is something that you’re not in the habit of doing, I’m happy to go into more detail about this sort of workflow.)
• Commit 30e149d (“Remove warnings from test suite”) - where have these warnings come from, and why are we fixing them here? Are they warnings that appear when building in Xcode 12, for example?

[lawrence-forooghian]

(Leaving this comment so that the review request is marked as actioned. My above comment is my review, but I had to leave it as a normal comment because it was too long for a review comment.)

[QuintinWillison]

Thanks @lawrence-forooghian! 😁 I don't disagree with anything you've said. We will get better at this.
(internal conversation expanding on this)