Added support to others protocols than application for subscriptions (eg email or sms)

[claustres]

Does not break API because a new subscribeWithProtocol method has been added but is called internally by subscribe with the default application protocol.

[evanshortiss]

Hi @claustres looks good, and more importantly - no breaking changes 👍

One thing I'm not sure about is how this fits with the goal of this module. For example, if a user uses subscribeWithProtocol(endpointArn, topicArn, 'sms', callback) how will we send a message to them? This module exposes the sendMessage function which looks like this.

This assumes we are sending to 'application' types and not 'sms' and therefore converts the message to APNS and GCM format. So I'm wondering if this module will work for sending sms or email types?

[claustres]

Well I already tested it by sending a message to a topic where SMS subscriptions have occurred. In this specific case the endpoint you provide when subscribing the user is the phone number instead of the ARN. I used the default message format with a top-level default key and it worked. As far as I understand if the message structure used is JSON it can cover all the platforms by sending something like what is presented here.

If you already send platform specific data in the message your functions let it untouched so fine. Otherwise I am not sure that what you do is necessary if you set a default root key because you simply repeat the same thing in each specific platform entry. These entries should be used to insert additional options such as collapse_key and not send the same message IMHO.

However we probably need to perform more testing. Indeed I strongly believe your module could at least support SMS because it is mobile-oriented. I agree email or lambda is more farther than the initial objective of your module but the nice thing with AWS SNS is that it unifies all protocols under almost the same API. Let me know what you think.

Regards

[evanshortiss]

Well I already tested it by sending a message to a topic where SMS subscriptions have occurred. In this specific case the endpoint you provide when subscribing the user is the phone number instead of the ARN. I used the default message format with a top-level default key and it worked.

That's interesting. So it "just worked". Here's how I think it worked - I just read the code for each conversion function in this module and can see that if you set the opts.platform argument to SUPPORTED_PLATFORMS.ANDROID then I can see that this will work, because of this code here checking if default exists. If default does exist then your message will send without any changes. I assume this is why you are able to do this if using SUPPORTED_PLATFORMS.ANDROID, since iOS type will force the Object to look like either:

{
  APNS: messageJSON
}

or

{
  APNS_SANDBOX: messageJson
}

If you already send platform specific data in the message your functions let it untouched so fine. Otherwise I am not sure that what you do is necessary if you set a default root key because you simply repeat the same thing in each specific platform entry. These entries should be used to insert additional options such as collapse_key and not send the same message IMHO.

I'm not sure I understand this. Do you mean the formatting is no longer necessary, and that passing default is sufficient regardless of the underlying protocol? I think it was necessary when I originally wrote the module (iOS is a little picky), but perhaps not anymore which is great as a unified API like you mention.

Indeed I strongly believe your module could at least support SMS because it is mobile-oriented

Agreed. I think the correct solution here might be to update SUPPORTED_PLATFORMS in the code here to include an SMS type - this would then modify the sendMessage behaviour to not format for push notification services, but instead SMS. We'd also need to consider how these options in the AWS SDK can be passed.

What do you think?

[claustres]

Yes you are right I only use android for now.

According to this the specific message payload is only used if the default one is missing so it does not seem to be mandatory except for additional attributes.

Not sure about sms as a new platform, this might be confusing with aws sdk where it is a protocol. Or we should replace 'platform' by 'protocol' and create a 'push' protocol. It could manage all the underlying platforms specified at creation. I did a code like this on top of yours because you usually want to target all platforms.

[claustres]

You were right, due to my specific app flow I did not realize I could not send simultaneously SMS + Push on Android. To do so the message has to be structured like this to target simultaneously different protocols:

{
"default": "message", 
"email": "message", 
"sms": "message", 
"APNS": "{\"aps\":{\"alert\": \"message\"} }", 
"APNS_SANDBOX":"{\"aps\":{\"alert\":\"message\"}}", 
"GCM": "{ \"data\": { \"message\": \"message\" } }", 
"ADM": "{ \"data\": { \"message\": \"message\" } }"
}

So this work with my change as far as you give this message structure. Otherwise I would suggest that your converter function merge the input message when adding a GCM key for instance instead of starting from an empty container here. Or you always add a sms key as well so that if some phone numbers are registered to the topic they will be notified.

To manage SMS attributes we should only expose a specific function calling AWS SDK's one just like you do for endpoints here.

Another great benefit your module can offer is also a promise-based API that is not availalbe through the Amazon SDK.

[evanshortiss]

@claustres I was actually planning to update it for Promises and some TypeScript definitions very soon. No ETA though - too busy sometimes 😄

For my specific use case I never had the need to send many formats in one payload, but I see the use case and AWS even discuss here 👍

Or you always add a sms key as well so that if some phone numbers are registered to the topic they will be notified

This is nice and convenient, but it can add some inefficiency due to the larger payloads. I think maybe in a future version we could have something like this:

const SNS = require('sns-mobile')

const sender = new SNS.Sender({
  // default protocols to include
  protocols: [SNS.protocols.GCM]
})

const msg = {
  // override defaults so we send to sms AND google (known as FCM now)
  protocols: [SNS.protocols.SMS, SNS.protocols.GCM]
  message: 'Hi there!'
}

sender.sendMessage(targetArn, msg)
  .then((response) => console.log('success', response))
  .catch((e) => console.log('darn, something went wrong', e))