feat(framework): add subscriber data mapping GRW-11 #6031
Conversation
| @@ -5,4 +5,7 @@ export type Subscriber = { | |||
| email?: string; | |||
| phone?: string; | |||
| avatar?: string; | |||
| data?: Record<string, any>; | |||
There was a problem hiding this comment.
Can we strongly type data? I don't think we shouldn't have any interfaces exposed to the Framework consumer, it weakens the type safeness of their Novu integration. We should probably explore type declaration merging on this data object instead.
As a related follow-up, what is the origin of data on the subscriber?
There was a problem hiding this comment.
It appears to be this data property on the Subscriber's POST /v1/subscriber request. Given that such data lookups should now happen during the code-first Workflow execution to ensure maximum data freshness, I think we can remove this attribute entirely.
There was a problem hiding this comment.
I do think it can serve a different purpose with v2, but let's touch base on this over a coffee later. Want to brainstorm a little on this
There was a problem hiding this comment.
So citcle back on this,
I do still see some usecase where users might want to persist some subscriber entity related data that is part of the notifications domain.
Some examples currently where customer have some complex preference logic and they want to persist it inside of the subscriber and later use to manipulate the workflow.
I think it still very usefull ability, to not force our customers worry about storing this notification related custom data somewhere on their servers
There was a problem hiding this comment.
Regarding the any, it makes sense. Where would you provide such a global type?
What changed? Why was the change needed?
https://github.com/novuhq/packages-enterprise/pull/164
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer