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

Refactor/spring cleaning #234

Closed hypesystem closed 8 years ago

hypesystem commented 8 years ago

:star: Comments and discussion welcome. :star:

This is some refactoring and code sanitation of the Sender class.

I am very open to feedback. Am I going in the right direction?

Before merging this, we should merge the following into this branch:

eladnava commented 8 years ago

@hypesystem Great job!

Looks good to me -- ran the tests (removed a stray console.log) and they pass =)

However, when I ran the examples/notification.js, I got a TypeError because the options array is null, on this line: https://github.com/ToothlessGear/node-gcm/pull/234/files#diff-c890425a040068c3e899d1f10233671bR75

The options is set to null here: https://github.com/ToothlessGear/node-gcm/pull/234/files#diff-c890425a040068c3e899d1f10233671bR18

I set it to {} and the error seems to have been fixed. Do you want me to commit the fix to the branch?

One question though -- why use nextTick and not invoking the callback with errors directly?

hypesystem commented 8 years ago

@eladnava Weird!

I thought it would enter the earlier case in cleanOptions (the one checking typeof options != "object"), which returns early: https://github.com/ToothlessGear/node-gcm/pull/234/files#diff-c890425a040068c3e899d1f10233671bR69

Turns out that typeof null; is "object". Great. Will fix :smile:

hypesystem commented 8 years ago

Fixed in https://github.com/ToothlessGear/node-gcm/pull/234/commits/58dcde17b95eed600fd23c3f0ad345ee77471921

Feel free to merge if you are happy with the code :smile:

eladnava commented 8 years ago

Haha, yeah, JS is a bit messed up to say the least. =)

eladnava commented 8 years ago

Seems well, all tests pass and I was able to send a notification. Well done! :)