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

Background push notification docs #745

Open
mattheworiordan opened this issue Oct 16, 2019 · 7 comments
Open

Background push notification docs #745

mattheworiordan opened this issue Oct 16, 2019 · 7 comments
Assignees
Labels
api-reference Anything that appears below the API reference blue fold content-request A request for new content, as opposed to changing/fixing existing content

Comments

@mattheworiordan
Copy link
Member

mattheworiordan commented Oct 16, 2019

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

@mattheworiordan mattheworiordan added content-request A request for new content, as opposed to changing/fixing existing content important api-reference Anything that appears below the API reference blue fold labels Oct 16, 2019
@ricardopereira
Copy link
Contributor

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:

  • Apple has started discontinuing support of TLS v1 for their Push API
  • APNS Priority must be set to 5 for background notifications! apns-priority header value should not be 10 (default and it means immediate delivery). Value of 5 is now required for all “content-available” notifications.
  • The Binary Provider API , legacy binary interface, which Apple wants to deprecate in prol of the HTTP/2-based API.

More references:

@paddybyers
Copy link
Member

paddybyers commented Oct 25, 2019

@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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

mattheworiordan commented Oct 27, 2019

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
Copy link
Member

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-reference Anything that appears below the API reference blue fold content-request A request for new content, as opposed to changing/fixing existing content
Development

No branches or pull requests

5 participants