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 206 forks source link

promisify node-gcm #264

Closed dan-perron closed 8 years ago

eladnava commented 8 years ago

Lots of changes here. I'm not entirely familiar with the v1 code, @hypesystem what do you think?

hypesystem commented 8 years ago

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 send's internal call in new Promise if no callback is given.

@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 new Promise instead, as well :smile:

dan-perron commented 8 years ago

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.

hypesystem commented 8 years ago

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?

eladnava commented 8 years ago

@dan-perron Nice, clean solution! I prefer it this way as well. Excellent work.

@hypesystem LGTM.

dan-perron commented 8 years ago

Squashed.

hypesystem commented 8 years ago

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!

dan-perron commented 8 years ago

I don't see a section of the README for contributors. What am I missing?

dan-perron commented 8 years ago

Ping?

hypesystem commented 8 years ago

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

dan-perron commented 8 years ago

Done!

Hmmm... merge conflict. Guess I'll rebase quick.

dan-perron commented 8 years ago

Ok, should be ready now.

hypesystem commented 8 years ago

Cool! Thank you so much for the time you put into this :smile: this will be out with the next release candidate of v1.