Adding Support for User Notifications

[brianbowden]

I understand this is massive, and I apologize for the mess this might make for existing pull requests. However, I've spent a couple weeks extending node-gcm to support GCM's user notifications, and through blood, sweat, and tears, I've developed a helpful set of features. While this obviously does not include the necessary XMPP client to fully support upstream notifications from device to server, device-to-device push notifications will now be possible via notification key. I'd heavily suggest reading the extensive guide I wrote to help understand user notifications and fill in the gaps within the official docs. Existing official docs are fairly poor and don't address significant pitfalls.

I've updated the changelog and usage section. While I still need to write some tests for creating and maintaining notification keys, I wanted to get this out here for discussion.

[hypesystem]

This looks interesting! I will give it a proper look as soon as I have the time :)

[hypesystem]

I feel like at least a couple of the latter commits are not meant for this library. You need to fix that before I will consider a merge.

A lot of your commits are very large and hard to overview, especially without much descriptio nor reasoning from your part. The first commit, in particular, falls victim to this. If you could add some logic behind your decisions, that would be much appreciated.

[hypesystem]

In addition, your PR contains a lot of code not immediately related to your stated title. You have no justification for these, and that makes it hard to consider them in any kind of serious way, without making a lot of assumptions.

[hypesystem]

You have some not-very-flattering product placement in there, as well. Referring to something written by you elsewhere, rather than striving to make the README stand on its own is hard to see as anything other than a sign of bad faith.

I am referring to this (in README.md):

If you are new to user notifications, it is highly recommended that you read Brian Bowden's guide for implementing User Notifications which explains some key points unmentioned in the official documentation.

[hypesystem]

Some general code feedback:

• You have an Operation and a Notification that can be created in an invalid state, but depend on the user checking isValidX to verify them, before sending them off. It would make more sense to verify that the arguments given to a constructor are valid in the constructor. Keep the error-ing close to where the fault was committed, not way down stream from there.
• You have extracted some of the functionality of Sender into a SenderBase to enhance extendability. Good idea, but you do this with Javascript's pseudo-inheritance. Generally, I would recommend to prefer composition over inheritance, and inject it as a dependency, rather than inherit from it.

[hypesystem]

You add some tooling (grunt), which will generally make it easier to be a contributor. Great stuff!

[hypesystem]

Overall, it seems you have already decided to branch out, and your project does not only conflict with pending PRs, but also the generally intended direction of the project.

A word of advice: If you plan on contributing to a project in the future, it would be really nice if you keep the project maintainer(s) in the loop about this. Partake in discussions on issues, add pull requests early and frequently, so the direction can be discussed properly. Adding One Big Pull Request(tm) with much more work than what is noted in the description and expecting it to be merged is unrealistic.

[hypesystem]

I am interested in knowing whether you are interested in working through (or rebutting) the issues I find with the PR, or whether I should close the issue.

[brianbowden]

Firstly, I apologize for suggesting the blog plug. You are right, and that came across as more of a self-promotion than I intended. It does contain some helpful info, but there's probably a better way to handle that in a community-driven project.

Thanks for the advice and the feedback on the poor commits (I normally work alone or with a small team that discusses code in person); I definitely need to improve that in the future. Unfortunately, I think I will need to continue taking this in another direction. It was written primarily to support efforts for other projects, and I will need to build out features with those in mind. I just thought "Hey, I'll contribute to the parent project!" without thinking of the ramifications to the community effort, so I apologize for wasting your time.

Thanks for your contributions to this project; it's definitely helped people like me!

[hypesystem]

Okay. Thank you for your time and effort :-)