Sender.prototype.send() making bad assumption on recipient

[alemonmk]

It seems there's no documentation telling me how to supply recipient token when I need to send unicast message, so I have a look into lib/sender.js. From what I read, if I specify a single token (i.e. a string), it gets converted into a single-element array, and then call sendNoRetry().

In lib/sender.js/send()

    if(typeof recipient == "string") {
        recipient = [recipient];
    }
    else if (!Array.isArray(recipient) && typeof recipient == "object") {
        // For topics, passing them to sendNoRetry() as if they were registration tokens
        // will put them in the "to" value of the JSON payload, which what we want
        var o = extractRecipient(recipient);

        if (o.err) {
            return callback(o.err);
        }
        else {
            recipient = o.recipient;
        }
    }

sendNoRetry() will put the single-element array into registration_ids which is discouraged:

Use this(registration_ids) parameter only for multicast messaging, not for single recipients.

PS. Is passing a { registrationTokens: 'a string token' } sends a unicast job?

[eladnava]

Hey @alemonmk, Thanks for bringing this up, and sorry for the delayed response. You have a very good point there regarding unicast messaging.

While sending a notification to one registration ID as a multicast message works perfectly fine, it would be better if we adhered to the GCM documentation, which specify that we should use the to parameter instead, if only sending to one recipient. @hypesystem, what do you think?

Regarding passing { registrationTokens: 'a string token' } -- that won't work. That will either crash the app or return an err to your callback.

[hypesystem]

I agree that we should probably use the to field whenever possible :smile:

[eladnava]

@alemonmk When PR #190 will be merged, node-gcm will return an error if you try to pass in { registrationTokens: 'a string token' }, and won't crash the app.

The right way to pass in a single registration ID as of today is like this:

sender.send(message, 'abcd1234', function(err, response) {
  if(err) console.error(err);
  else    console.log(response);
});

I will submit a PR soon that will change the behavior of providing a single registration ID (to use the to field instead of a single-element registrationTokens array).

[eladnava]

@hypesystem How about we add a to recipient key alias as well? (In addition to registrationTokens and topic, for example).

[alemonmk]

Just wondering, wouldn't make this if block

    if(typeof recipient == "string") {
        recipient = [recipient];
    }

does nothing more straight forward?

[eladnava]

@alemonmk Thanks for pointing that out. Passing in a recipient as a string should also be treated as unicast messaging.

I've added it to the PR as per your suggestion: https://github.com/eladnava/node-gcm/commit/d3bc8d73f33c3dcbfc38b1012dd7e1507a6334b3

[alemonmk]

I found out that eladnava/node-gcm@8cdc4d9 probably would complicate things up in eladnava/node-gcm@703441d. With this in mind, I suggest that having the parameter recipient required to be an object, which holds one of the following keys: to, topic or registrationTokens (bring back notificationKey and registrationIds for the sake of compatibility). A breaking change, but I think it is cleaner.

[eladnava]

@alemonmk We should still make it possible to provide a string as the recipient object, for backward compatibility.

However, the recipient types documentation doesn't list the possibilities of providing a string or array as they are not recommended (we'd prefer if people used the recipient keys instead).

Regarding adding a to recipient key, waiting for @hypesystem's response:

@hypesystem How about we add a to recipient key alias as well? (In addition to registrationTokens and topic, for example). https://github.com/ToothlessGear/node-gcm/issues/187#issuecomment-164052864

[alemonmk]

I see, thanks for clarification :)

[hypesystem]

@eladnava :+1: sounds great :smile: for some reason I actually thought we already supported a to recipient key, but it looks like we don't.

Once we've added the to key, this issue can be closed :smile:

[eladnava]

@hypesystem @alemonmk Waiting on #190 to be merged because it introduces a change in the way we check for recipient keys.

[eladnava]

@hypesystem Submitted the PR =)

[hypesystem]

And it's merged!