search

ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging
https://github.com/ToothlessGear/node-gcm
Other
1.3k stars 208 forks source link

V1 no send no retry #248

Closed hypesystem closed 8 years ago

hypesystem commented 8 years ago

Note: this merges into v1 #238

This refactors the sender code a whole bunch, and removes the previously exposed sendNoRetry method. The same effect can now be achieved with send(msg, recipient, { retries: 0 }, callback).

eladnava commented 8 years ago

@hypesystem excellent work!

Tested it and works fine.

One minor thing, the package doesn't detect an invalid registration token currently if GCM returned InvalidRegistration for it, the package only checks for Unavailable: https://github.com/ToothlessGear/node-gcm/blob/7e556990ac9be287819be290fef57264dc7078e9/lib/sender.js#L155

Is this intentional? All tests pass, by the way. Merge when ready.

hypesystem commented 8 years ago

This is not something I have made a deliberate choice about now. It is how it was before...

I think the reasoning is that only unavailable registration tokens should be retried --- if the registration token is invalid, there is no need to retry. In the returned result, the invalid registration ids should still be present :smile: we don't have a test for it, but I definitely think we could add one. I might do that later !

eladnava commented 8 years ago

@hypesystem I'm thinking the package shouldn't be retrying an InvalidRegistration, but returning it within the array of registration tokens that are no longer valid and need to be removed by the developer.

hypesystem commented 8 years ago

Absolutely! That's basically under the point of #238 that is about better responses :smile: gonna merge this now and will look at the responses later

govardhanaraoganji commented 8 years ago

@hypesystem and @eladnava have doubt, what is the use of retries, because of the retries param we are continuously hitting the server and some delay in response.

what we are going to achieve with retries ?

eladnava commented 8 years ago

@govardhanraoganji the GCM endpoint is bound to fail sometimes (due to an internal server error, for example) and so the GCM request should be retried in this scenario as it will likely succeed next time.

We do not retry a request that has failed due to user error, e.g. bad authentication, bad registration token, etc.

hypesystem commented 8 years ago

@govardhanraoganji we also retry the message for any registration tokens for which it was not delivered. Just in case it was a momentary outage.

govardhanaraoganji commented 8 years ago

@eladnava and @hypesystem thank you guys, your support was un countable.