[Feedback] NIOAPNS: NIO-based Apple Push Notification Service

Server Proposals
19

Thanks @kylebrowning, this looks really good. I just have feedback regarding the types left, this is mostly expanding a few points from my previous feedback.

ByteBuffer as the buffer type

This package uses Data as the data type for buffers everywhere. I totally think we should support Data but we should also support ByteBuffer. In the NIO ecosystem, you most likely already have ByteBuffers, now if you use this package, you'd go ByteBuffer -> Data -> ByteBuffer to actually write it to the wire. So we have two unnecessary conversions here.

I think all types should be changed to use ByteBuffer as the main type and have convenience functions that allow the users to use Data too. That would make nothing slower but will make everything that uses ByteBuffer (which is what you get from the network) massively faster.

Top-level types

I think we have too many top-level types here. This is a (hopefully) complete list of the types we have:

NIOAPNS module

  • public struct APNSConfiguration

  • public final class APNSConnection

  • public enum APNSErrors

  • public enum APNSResponseError

why do we need this? This should be part of or nested inside of APNSErrors

  • public enum APNSTokenError

why do we need this? This should be part of or nested inside of APNSErrors

  • public struct APNSError

why do we have this? This should be part of or nested inside of APNSErrors

  • public enum APNSSoundType

Couldn't this be nested in APNSNotification?

  • public protocol APNSNotification

  • public struct APNSSoundDictionary

this one too?

  • public struct APSPayload

  • public struct Alert

This name is way to general, we should nest this into something too.

  • public struct APNSRequestContext

what does this do, does it really need to be a top-level type?

NIOAPNSJWT module

  • public enum APNSSignatureError

Why is this in the NIOAPNSJWT module?

  • public class DataSigner

can we nest this in a enum APNSSigners {}?

  • public class FileSigner

this one too?

  • public struct JWT

  • public protocol APNSSigner

Why is this in the JWT module?

  • public enum SigningMode

I think we should also nest this into something

1 Like