Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

bill-kerr
Copy link
Collaborator

Allows for notification handlers to subscribe to multiple topics.

Copy link

@SeriousBug SeriousBug left a 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

return pascalCase(
`${handler.definition.topicName}-${handler.definition.name}`,
);
return pascalCase(`Notification ${handler.definition.name}`);

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?

Copy link
Collaborator Author

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

@s-fletcher s-fletcher removed their request for review December 12, 2023 23:04
@loganhuskins
Copy link

@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?

@bill-kerr
Copy link
Collaborator Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants