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

InvalidRegistration when sending to topics #278

Closed sapostolou closed 6 years ago

sapostolou commented 7 years ago

I'm trying to send a notification to a topic but I get the InvalidRegistration error.

This is the message's code:

var fcmMesssage = new gcm.Message({
        collapseKey: 'key1',
        priority: 'high',
        contentAvailable: true,
        notification: { 
            title: 'My app',
            message: 'New stuff',
            icon: "myicon",
            tag: 'mytag'
        }
    });

and the sender's code:

sender.send(fcmMesssage, { condition: '\'mytopic\' in topics'}, function (err, response) {  
        if (err){
            console.log(err);
        }
        else{
            console.log(response);
        }
    });

I had no problem sending the following message through the HTTP API:

{
    "condition":"'mytopic' in topics",
    "collapse_key":"demo",
    "priority": "high",
    "content_available":true,
    "notification":{
        "title":"My app",
        "body":"New stuff",
        "tag":"demo2"
    }
}
sapostolou commented 7 years ago

I replaced condition: '\'mytopic\' in topics' with condition: '\'mytopic\' in topics || \'mytopic2\' in topics' and it worked.

I did that because of this line in sender.js.

It will work for me anyway since I will always be using an actual condition but I think that it should be able to work without an actual logical AND or OR.

eladnava commented 7 years ago

@taroutatsou Thanks for reporting this!

@hypesystem I think we should modify extractRecipient to pass the type of recipient extracted to the callback and not just the recipient value here. That way we don't have to test the value for a regular expression at all, and we'd already know it's a PubSub topic notification.

What do you think?

hypesystem commented 7 years ago

The alternative is fixing the regex (which seems like a smaller change), but I don't really mind -- either solution is good :smile:

eladnava commented 7 years ago

To be honest I don't think that the regex detection is a robust enough solution to begin with 🤕

Patching it up now will probably lead to more patching later.

eladnava commented 7 years ago

@taroutatsou Would you like to submit a PR? 😄

eladnava commented 6 years ago

Fixed via #312.