Pass on response body with error information (similar to #79)

[lwille]

While #79 adds some more sophisticated error handling, we were just interested in the response body in case of an error 400 (resBody was "Number of messages on bulk exceeds maximum allowed (1000)" in our case).

[hypesystem]

Who are "we"? Why are you only interested in responses on error 400? Might other people not be interested in other error codes?

I have one problem with this change: it is a breaking change. According to Semantic Versioning this requires a major version bump. There are plenty of other changes we would like before this is done. In fact, that's what the v1 branch is for.

Why do you need it returned in the callback? Using the DEBUG flag should now give you more information. Anyway, it's bad practice to react to the contents of a reason string (as they may change without version bumps).

---

A note on style: The second argument to a callback is a result. In the case of an error, no result is produced. The response text is not a result, but a reason for the error, and should be returned as part of the first argument, ie:

//THIS
callback({
    statusCode: statusCode,
    reason: reason
});

//NOT THIS
callback(statusCode, reason);

[hypesystem]

I think a better solution to this issue would be for the library to automatically chunk calls to GCM, so they do not exceed the 1000-message limit. I created an issue for this: #83

[lwille]

I can't just switch on debug in production. I was seeing "error 400" with no further information; whereas the Google documentation states "The exact failure reason is described in the response and the problem should be addressed before the request can be retried."

We both agree that an unspecific error has to be escalated.
Of course any other unspecific error is of equal importance; it was just the 400 that occured in production: a user had registered ~4000 device tokens and I wouldn't have discovered this without the "unknown error 400" :)

I like your extended error format proposal, but I disagree on your semver concerns though. If you look at README.md, you have to assume that the 2nd param contains the response regardless of the 1st.

Anyway, I would have probable fully read your merge request if it was a single commit. Please squash them.

[hypesystem]

I agree that running with debug in production is a bad idea. But to be honest, all bad requests should have been mitigated in your code before it reaches production. Google do a great job of hiding it, but they have actually documented the limit:

Also, GCM allows you to attach up to 1,000 recipients to a single message, letting you easily contact large user bases quickly when appropriate, while minimizing the work load on your server.

You are right that an unspecific error cannot be escalated, but a 400 error can never be escalated. It means your request was wrong, ie. your code generates requests wrong. They should be right the first time around. I do think there is a usability error, hence #83.

My main argument remains that this is a breaking change. You don't seem to bite. Let me try and convince you (one last time): the result returned from sending is usually a javascript object. In your code, it is a string. Returning a string where an object is expected (or was previously the only valid response) is a breaking change.

Squashing commits is a whole other discussion, and I won't elaborate here. But if you want an overview of all the changes in a pull request, use the "Files Changed" tab: https://github.com/ToothlessGear/node-gcm/pull/79/files

[lwille]

Now I get your point.

Well, according to docs, 400 means something like "unparsable request", whereas in reality it was "just" too many registration ids.
A regular 400 can't occur in production if you always generate requests in the same way, but in this case Google is misusing 400 for an undocumented edge case.

Anyhow, I'd be glad to have this improved error handling in the next version.