-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
feat: preUnsubscribe handler #605
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 709159642
💛 - Coveralls |
@@ -69,6 +69,7 @@ function Aedes (opts) { | |||
this.authorizePublish = opts.authorizePublish | |||
this.authorizeSubscribe = opts.authorizeSubscribe | |||
this.authorizeForward = opts.authorizeForward | |||
this.preUnsubscribe = opts.preUnsubscribe |
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.
You should add a defaultPreSubscribe
function and add it to defaultOptions
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.
Ah, yes, good point. Thanks, done.
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.
Should that allow some async stuff like the others do? maybe adding a callbackk to it?
35986a5
to
1436c79
Compare
@@ -69,6 +69,7 @@ function Aedes (opts) { | |||
this.authorizePublish = opts.authorizePublish | |||
this.authorizeSubscribe = opts.authorizeSubscribe | |||
this.authorizeForward = opts.authorizeForward | |||
this.preUnsubscribe = opts.preUnsubscribe |
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.
Should that allow some async stuff like the others do? maybe adding a callbackk to it?
The main question is what's the use case of modifying subscription topic in |
@gnought You mean |
@gnought the example I'm providing in the description is the use-case I need it for: I need to force clients into their own namespace, without the clients needing to know about their namespaces. Clients may subscribe to |
4fbc84c
to
6deb055
Compare
@robertsLando I've made |
6deb055
to
c00dc6c
Compare
Invoked then a client unsubscribes from topics. Just like authorizeSubscribe, this function allows modifying the topics because executing the unsubscribe. This is needed in cases where topics are modified in authorizeSubscribe. For example: ```js aedes.authorizeSubscribe = (client, subscription, callback) => { // overwrite subscription: force client to its namespace subscription.topic = `/${client.id}/${subscription.topic}`; callback(null, subscription); } aedes.preUnsubscribe = (client, packet, callback) => { // overwrite unsubscriptions: force client to its namespace for (let i in packet.unsubscriptions) { packet.unsubscriptions[i] = `/${client.id}/${packet.unsubscriptions[i]}`; } callback(client, packet) } ``` Includes fix-ups: - Update docs/Aedes.md Co-authored-by: Daniel Lando <[email protected]> - added defaultPreUnsubscribe - made preUnsubscribe async like authorizeSubscribe
c00dc6c
to
1e96245
Compare
I believe you also hook |
yes, exactly, @gnought . |
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.
Can you please add a unit test?
How about if we add the support of client isolation? Will it fit in your case? @chfritz |
Just like your use case, we prepend some client-specific string in client topics. |
I see. Yes, I think that would work for me. |
Invoked then a client unsubscribes from topics. Just like authorizeSubscribe, this function allows modifying the topics before executing the unsubscribe. This is needed in cases where topics are modified in authorizeSubscribe. For example: