Closed eladnava closed 8 years ago
There should be a test for the registrationIds
key, as well, for backwards compatibility :-) Other than that, it looks good, and I am ready to merge :)
@hypesystem how would you like to test this?
The current test utilizes registrationTokens
and checks whether the registration_ids are set correctly (we need to rename that field as well).
it('should set the registration ids to reg ids explicitly passed in', function () {
var sender = new Sender('myKey');
var m = new Message({ data: {} });
var regIds = ["registration id 1", "registration id 2"];
sender.sendNoRetry(m, { registrationTokens: regIds }, function () {});
var body = JSON.parse(args.options.body);
expect(body.registration_ids).to.deep.equal(regIds);
});
Would you like to duplicate this test and replace the registrationTokens
parameter with registrationIds
? Or did you have something else in mind?
I would simply duplicate the test, as you suggest :-)
The registration_ids
field is the one used in GCM, so we can't really change the name of that: https://developers.google.com/cloud-messaging/http-server-ref#table1
@hypesystem Excellent, added an additional test. :+1:
By the way, I found some other references in the package to "registration IDs", and use of a variable called regIds
-- would you like me to refactor those? (Leaving the registration_ids
GCM parameter intact, of course)
Yeah, sure, let's get them all for consistency :)
Excellent, renamed all occurrences I could find and split up the modifications into 4 commits (1 per file). Let me know what you think @hypesystem :+1:
Looks good! Thanks for the great work :-) :+1:
Create an alias for
registrationTokens
as per the new GCM terminology "registration tokens" instead of "registration IDs".It behaves the same as
registrationIds
. We decided to leaveregistrationIds
intact to maintain backward compatibility.Also, update the "no recipient" error message, as recipients can be both tokens and topics now.
Finally, incorporate the new alias in the senderSpec.js test.
References #160.