-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow multiple topics for notification handlers #1
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one concern about the handler names
extract/extract-handlers.ts
Outdated
return pascalCase( | ||
`${handler.definition.topicName}-${handler.definition.name}`, | ||
); | ||
return pascalCase(`Notification ${handler.definition.name}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you leave a backwards-compatibility option here so handlers with topicName
continue to get the same name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh good call, yes can do
@bill-kerr what's going on with this? I just got a notification for it. Was it that you added the beta tag on due to my questions about Draper from the other day? |
I pushed up the package version change since you couldn't find it in code anywhere |
Allows for notification handlers to subscribe to multiple topics.