faressoft / onesignal

A Wrapper for OneSignal Push Notification Delivery for Node.js. Provides the minimal functionality to send notifications for iOS and Android.
MIT License
20 stars 7 forks source link

Add ability to pass additional parameters to create_notification api #1

Open mkuklis opened 7 years ago

mkuklis commented 7 years ago

@faressoft this should allow to pass additional parameters to the create_notification api https://documentation.onesignal.com/reference#create-notification for example headings ,subtitle,template_id...

faressoft commented 7 years ago

Great, does this change allow also to pass segments and filters params ?

faressoft commented 7 years ago

Can we create an extra argument instead, it seems a little bit complicated, like this:

We can break our api and release a new major release for this change.

mkuklis commented 7 years ago

@faressoft sounds good. The issue here is that the message can be optional because you can provide a template_id (that's the issue I ran into). Perhaps a better option here could be to introduce another method?

faressoft commented 7 years ago

@mkuklis another method for what ? what are the arguments ? any suggestions for the methods names ?

faressoft commented 7 years ago

@mkuklis how about allow sending template_id in the message argument:

message

Do you think it is acceptable doing it that way ?

mkuklis commented 7 years ago

@faressoft One method for sending notifications with just a message like you had it before. And a new one which takes object with multiple parameters.

With your proposed API:

If I want to send a notification by template_id I would have to call:

createNotification([ids], null, data, {template_id: 'id'})

which is kind of complicated too.

mkuklis commented 7 years ago

@faressoft template_id is also a string so we won't be able to distinguish between message or template_id.

fbatiga commented 7 years ago

I second this change !