feat: Move notification config out from hard-coded in subscriber #7522
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
| * ```js title="medusa-config.js" | ||
| * module.exports = { | ||
| * projectConfig: { | ||
| * notifications: [ |
There was a problem hiding this comment.
Two questions that came to mind:
- How would something like localization be supported? So being able to define a template per locale
- How would you shape the
data? If I want specific values to map from something indatato what my email template expects. For example, "unwrapping" deep relations in the data to live at the root level.
There was a problem hiding this comment.
- What we can do is:
- pass a
localeto the notification module - have the
templatefield be astring | Record<language, string>, and if it is a record, select the correct template based on thelocalepassed as data.
- Currently we use
lodash.get, to which you can doget(data,order.shipping_method[0].id)` to compose your data as the template expects it. We could do a more sophisticated templating engine, but I think this should be enough for now.
I think it's important to keep in mind that we won't try to solve every possible use-case with this approach, as in that case it might be easier for people to implement their own subscriber. We are simply trying to make the simple use-cases easy to do, without any code changes.
WDYT?
There was a problem hiding this comment.
@sradevski, I just played around with this to get proper SendGrid emails sent out when placing orders.
The thing that concerns me with this approach is that the subscriber is not really useful because of how we generally pass data in events. Let me try to explain.
Typically, we only pass the ID of a given resource that relates to the event, e.g. when completing an order the event will look as follows:
{ name: "order.placed", data: { id: "order_1234" } }We do this to 1) avoid bloating the size of the job and 2) encourage the consumer of the job to fetch fresh data instead of relying on what was stored with the job. So with this approach, what we'll end up sending to the notification provider is just the ID of the resource, because that's the only data we have available in the subscriber. And an ID is not sufficient to use for notification purposes
Of cause, we could rethink how we generally send events such that they actually include enough data to construct a meaningful notification, but I am not sure this is the right approach given the reasons mentioned above.
So to sum up, the idea of having a generic subscriber is great, but I am afraid developers will end up having to implement their own anyway. I am not saying we should go with one or the other right away, but I wanted to flesh out my thinking, so we could continue the discussion.
There was a problem hiding this comment.
@olivermrbl in a typical event-driven architecture you send the entire payload, so that others don't need to rely on REST (and are therefore decoupled from other services). That being said, there are benefits to just sending the ID, so I don't think we necessarily need to change anything right away in that part.
Now, obviously if it's only the ID the common subscriber makes less sense. We can of course make it work by doing a fetch since we know the event object and the ID, but I don't think it's necessary to add that complexity.
Now, if we don't go with this generic subscriber, how will the default notifications be implemented? We'll just have a bunch of subscribers shipped by default, and then for each subscriber have something in the config to specify the template?
There was a problem hiding this comment.
in a typical event-driven architecture you send the entire payload
I am not opposed to adding more data to the event payload. We just have to figure out what and how much, since we probably don't want to go many levels deep in the relations by default. Also, maybe there are some fields that we don't necessarily want to share with all other services.
We can of course make it work by doing a fetch since we know the event object and the ID, but I don't think it's necessary to add that complexity.
Agree, don't think this should be the responsibility of the generic subscriber if we choose to go with this approach.
Now, if we don't go with this generic subscriber, how will the default notifications be implemented?
What I had in mind was that providers would, by default, define what events they would like to receive.
So the SendGrid provider would specify "I would like to receive notifications for order.placed, order.shipped, order.canceled, etc." (or a wildcard for all). Using the approach in this PR, the same configuration would require a project notifications config like so (please correct me if I am wrong):
{
event: "order.placed",
template: "",
channel: "email",
to: "email",
data: {
order_id: "id",
display_id: "display_id",
},
},
{
event: "order.shipped",
template: "",
channel: "email",
to: "email",
data: {
order_id: "id",
display_id: "display_id",
},
},
{
event: "order.canceled",
template: "",
channel: "email",
to: "email",
data: {
order_id: "id",
display_id: "display_id",
},
},
...which is a bit verbose.
Now, in order for us to achieve what I outlined above, the providers would need to be able to tell the notifications module what events they would like to subscribe to at load-time. The notification module would store this information such that every time we create a new notification, we look up what providers are subscribed to the notification and call .send(...) for each.
Let me know if this makes sense. And if so, what you think.
There was a problem hiding this comment.
@sradevski, here's a proof-of-concept of what I had in mind: develop...poc/notifications
|
@sradevski is this still relevant? |
|
@riqwan yes it is, we need to just decide if this is approach we are taking, still waiting on decision from @olivermrbl or @srindom |
No description provided.