ably / docs

Ably Realtime API documentation
https://ably.com/docs
Apache License 2.0
20 stars 41 forks source link

Background push notification docs #745

Open mattheworiordan opened 4 years ago

mattheworiordan commented 4 years ago

Our documentation does not make it clear how background notifications (data) work with iOS and Android.

We have hit some issues recently for customers, which highlights issues in our docos.

@ricardopereira @paddybyers can you post in your findings into this issues so that we can improve our docs. Related issue https://github.com/ably/realtime/issues/2699

┆Issue is synchronized with this Jira Task by Unito

ricardopereira commented 4 years ago

TL;DR: A new attribute is required on all APNS API headers.

@mattheworiordan @paddybyers Starting from iOS 13 and watchOS 6 (both latest OS versions) Apple requires the presence of the header apns-push-type for background push notification. The value of this header can be: alert, background, voip, complication, fileprovider and mdm. You can see more details in: Sending Notification Requests to APNs | Apple Developer Documentation.

I wasn’t aware of that change and that’s why the background notifications wasn’t reaching my iPhone when we were testing. Ably Realtime is not setting that HTTP header field. From the documentation:

To send a background notification, create a remote notification with an aps dictionary that includes only the content-available key…

Additionally, the notification’s POST request should contain the apns-push-type header field, with a value of background. The APNs server requires this header field when sending push notifications…

This header is "required when delivering notifications to devices running iOS 13". Furthermore "if the header is missing on required systems, APNs may delay the delivery of the notification or drop it altogether."

When a device receives a background notification, the system wakes your app in the background. On iOS it calls your app delegate’s application(_:didReceiveRemoteNotification:fetchCompletionHandler:) method.

Source of this information is available in Pushing Background Updates to Your App | Apple Developer Documentation.

BTW, there’s an old documentation page which doesn’t explain this change and I had to watch the Apple WWDC 2019 session about Background Pushes Advances in App Background Execution - WWDC 2019 - Videos - Apple Developer. Things that I learned from that video:

More references:

paddybyers commented 4 years ago

@ricardopereira yes, we have an internal issue for this (https://github.com/ably/realtime/issues/2699)

You can pass headers to APNS with a payload looking like:

{
  name: 'example',
  data: 'rest data',
  extras: {
    push: {
        apns: {
            aps: { 'content-available': 1 },
            'apns-headers': {
                'apns-push-type': 'background',
                'apns-priority': '5'
            }
        },
        data: { 'proto-data': 'Hello from Ably!' }
    }
  }
}

We need to decide when it's appropriate to add these headers automatically.

mattheworiordan commented 4 years ago

Thanks @ricardopereira for this nifo.

We need to decide when it's appropriate to add these headers automatically.

Yes, we should decide soon, because at present our docs don't cover this, and expecting our customers to read the APNS docs about background notifications (for default use cases) is not practical.

@paddybyers my preference is to continue to provide a high level API for developers to send data notifications with a payload like:

{
   extras: {
     push:  {
        data: 'this is it, it is portable across platforms'
     }
   }
}

Then if customers want to set apns settings explicitly, they can do, but it's not needed

paddybyers commented 4 years ago

my preference is to continue to provide a high level API for developers to send data notifications with a payload like:

...

Yes I agree with that. However, if the app includes any explicit apns headers, then I think they should then control the entire apns section. So, we add all transport-specific attributes necessary if they supply portable content; otherwise if they supply transport-specific content for some transport, we merge in the portable content, but don't add anything else automatically.

mattheworiordan commented 4 years ago

However, if the app includes any explicit apns headers, then I think they should then control the entire apns section

In theory I like this, but it does make it a little harder too. If for exampel, they simply wanted to bump the priority, it wouldn't be unreasonable to just do:

{
   extras: {
     push:  {
        data: {
           key: 'this is it, it is portable across platforms'
        },
        apns: {   
          'apns-headers': {
             'apns-priority': '10'
          }
        }
     }
   }
}

Saying that, perhaps a better approach is for us to now look at any other common fields that we think we want to make portable, and just include them as new attributes such as:

{
   extras: {
     push:  {
        data: {
           key: 'this is it, it is portable across platforms'
        },
        priority: 'normal' | 'high',
     }
   }
}

See https://firebase.google.com/docs/cloud-messaging/concept-options#setting-the-priority-of-a-message for a similar approach by FCM.

paddybyers commented 4 years ago

You've chosen an unfortunate example, because in a background notification you have to specify apns-priority': '5'.

I agree that the set of portable properties can be expanded over time.

However, remember that the reason that this is an issue is that iOS 13 has different rules from other versions; so ultimately we have to provide a route for developers to have full control, or we can end up in a situation where the rules change and they cannot construct a valid payload for a given target.

The alternative is that we invent some way that someone can suppress the auto-injected content, but that's going to be adding more complexity and more arbitrary rules; I do think the best way to provide full control is to do what I proposed, ie if the app includes any explicit apns headers, then I think they should then control the entire apns section.

mattheworiordan commented 4 years ago

You've chosen an unfortunate example, because in a background notification you have to specify apns-priority': '5'.

True :)

I agree that the set of portable properties can be expanded over time.

Ok, but priority is probably a sensible one to add now anyway. I appreciate priority is going to be "normal" for APNS always for data messages, but that's OK. The point is customers should be able to choose this.

However, remember that the reason that this is an issue is that iOS 13 has different rules from other versions; so ultimately we have to provide a route for developers to have full control, or we can end up in a situation where the rules change and they cannot construct a valid payload for a given target.

Sure, but sadly it is also now our responsibility to keep track of new iOS/Android releases, and make sure our portable versions continue to work, or report the issues to customers so that they are able to address the problem. I think we need to set up a notifications on iOS/Android releases moving forward to a) read the release notes to understand the impact, b) test against beta releases before the go to GA. Do you agree?

I do think the best way to provide full control is to do what I proposed, ie if the app includes any explicit apns headers, then I think they should then control the entire apns section.

Sorry, but I don't, because I think it's unintuitive. Typically, I am convinced developers will intuitively provide apns attributes when they want to customize the apns payload for a particular element. For example, it would not be unreasonable to expect a customer to want to specify apns-expiration. If they previously published data messages, and now wanted to add an apns-expiration, they would now need to read up on all the documentation about how to make that portable and work across all iOS versions, and will have to construct/duplicate the entire payload in the apns. This in turn makes this more brittle for customers because now the issues we have had, with a new release no longer being compatible, falls to them to keep track of this and test with each new release.

I think what would be more acceptable and intuitive, is that if any key of apns is provided, it's value replaces the existing value (if one exists) and never merges. So in the case of specifying the priority, they would need to do something like this to keep it working:

{
   extras: {
     push:  {
        data: {
           key: 'this is it, it is portable across platforms'
        },
        apns: {   
          'apns-headers': {
             'apns-push-type': 'background',
             'apns-priority': '5'
          }
        }
     }
   }
}

I appreciate that still means developers are still subject to some brittleness and additional work, but replacing the entire apns section IMHO is too drastic and unnecessary. I also don't think that is what we do now, so it would be a breaking change.