promisify node-gcm #264
promisify node-gcm #264
Conversation
|
Lots of changes here. I'm not entirely familiar with the v1 code, @hypesystem what do you think? |
|
I think this is a lot of changes. I would like for the code in the lib to keep using callback style, and the promise part being as small as possible, preferably an isolated part. This should be possible by wrapping @dan-perron I'm sorry you got so far and did so much before I mentioned this! I should have said it before. While I think it is a nice benefit to add promise support, it should not be the way the internals are written. Do you want to take another stab at it? I would like to hear your thoughts on the possibility of using |
|
I'm fine with that. I have a strong bias to using promises over callbacks everywhere, but this is your code. I'll update shortly. |
|
This looks great :) Could I get you to squash the commits to a single one (seeing as the first three contain changes that were basically reverted), so our history looks a bit nicer? Other than that, I think this is good to merge. Any input, @eladnava? |
|
@dan-perron Nice, clean solution! I prefer it this way as well. Excellent work. @hypesystem LGTM. |
|
Squashed. |
|
Neat! Sorry for the long wait --- have you added yourself as a contributor in the README? I can't seem to find it :-) As soon as you let me know on that point, I'll merge it! |
|
I don't see a section of the README for contributors. What am I missing? |
|
Ping? |
|
Wait, that was an error on my part. I meant in the package.json. https://github.com/ToothlessGear/node-gcm/blob/master/package.json#L6-L41 |
|
Done! Hmmm... merge conflict. Guess I'll rebase quick. |
|
Ok, should be ready now. |
|
Cool! Thank you so much for the time you put into this 😄 this will be out with the next release candidate of v1. |
No description provided.