notifme / notifme-sdk

A Node.js library to send all kinds of transactional notifications.
https://notifme.github.io/www/
MIT License
1.94k stars 149 forks source link

feat: add webhook provider #54

Closed Eywek closed 5 years ago

Eywek commented 6 years ago

Add a webhook provider to use custom webhooks. It sends a JSON to specified URL with event name and data.

BDav24 commented 6 years ago

Hi @Eywek, thanks for your PR! Could you tell me more about your use case? Because there is a problem if you want to declare webhooks for 2 different services (only declaring 2 providers with a fallback/round-robin strategy is currently possible). So maybe the lib needs a more generic change.

Eywek commented 6 years ago

Hi, I'm using this to send custom webhooks to differents endpoints according to the notification.

BDav24 commented 6 years ago

Well, that I could figure.

Eywek commented 6 years ago

Oh, so, what do you want to say with if you want to declare webhooks for 2 different services ?

BDav24 commented 6 years ago

@Eywek Let's say I want to call 2 services, I can't declare them as follow:

new NotifmeSdk({
  channels: {
    webhook: {
      providers: [{
        type: 'Service1',
        send: async () => { /* call service 1 */ }
      }, {
        type: 'Service2',
        send: async () => { /* call service 2 */ }
      }]
    }
  }
})

because service2 will not be called unless service1 fails, which is not what we want in this case.

So I'm thinking of a more generic:

new NotifmeSdk({
  channels: {
    custom: {
      service1: {
        providers: [{
          type: 'Service1',
          send: async () => { /* call service 1 */ }
        }]
      },
      service2: {
        multiProviderStrategy: 'fallback',
        providers: [{
          type: 'Service2',
          send: async () => { /* call service 2 */ }
        }, {
          type: 'Service2Fallback',
          send: async () => { /* call service 2 fallack */ }
        }]
      }
    }
  }
})

What do you think?

vmarchaud commented 6 years ago

@BDav24 We aren't sure what we need to modify in order to make it work as you suggested. Could you explain a little bit how we could implement that ?

BDav24 commented 6 years ago

I'd add a new type custom into src/providers/index.js and create a "factory" src/providers/custom/index.js that always returns new CustomProvider(config). config being of the form {[string]: {send: (CustomRequestType) => Promise<string>}} ({service1: ..., service2: ...}), with a required send function for each service. The CustomProvider (src/providers/custom/custom.js) constructor receives the config, and its send method receives an Object with no particular type (type CustomRequestType = Object). send then call each config.services.send.

vmarchaud commented 6 years ago

Okay so i think i get it, i will try to implement a custom provider (because anyway we will need it for our use case). However i think a generic implementation for webhook should be beneficial for everyone (it isn't specific to our use case), so do you mind accepting the both the webhook & custom provider (the later in in a other PR) ?

EDIT: I will obviously try to fix the behavior of the webhook provider to handle the case you mentionned

vmarchaud commented 6 years ago

After looking at the code, i don't see why in the case of

new NotifmeSdk({
  channels: {
    webhook: {
      providers: [{
        type: 'Service1',
        send: async () => { /* call service 1 */ }
      }, {
        type: 'Service2',
        send: async () => { /* call service 2 */ }
      }]
    }
  }
})

We don't get the same behavior, the code is implemented the same way as others. The default strategy for all providers is the Fallback one, right ? So its normal in your case that the second will only be called if the first fail.

BDav24 commented 5 years ago

We can now define custom channel providers: https://github.com/notifme/notifme-sdk/releases/tag/v1.9.0