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

Refactored Subscribe Handler #408

Merged
merged 5 commits into from
Feb 21, 2020
Merged

Conversation

gnought
Copy link
Collaborator

@gnought gnought commented Feb 5, 2020

No description provided.


// no messageId conditions:
// - restore sub, won't have messageId
// - programatically trigger subscribe without messageId
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this restore property come from? Maybe we could avoid setting it in the first place.

@gnought
Copy link
Collaborator Author

gnought commented Feb 6, 2020

I think it is best to use connecting flag #409 to replace restore property where defined in https://github.com/moscajs/aedes/blob/master/lib/handlers/connect.js#L179

@gnought
Copy link
Collaborator Author

gnought commented Feb 7, 2020

Users may hook broker client event to do a subscribe. The connecting is true during client event emits. In this case users cannot subscribe a topic if we use connecting to replace restore property. As we support queueing messages before CONNECT, I abandon this approach.

Previous restore is added as an item in packet object in handleSubscribe to handle restored subscription behaviour (https://github.com/moscajs/aedes/blob/master/lib/handlers/connect.js#L181), it is not safe if users inject this key. My approach is to remove it, and add one more arguments in handleSubscribe.

In addition handleSubscribe is refactored to handle normal and restored subscription separately. Simpler and readable.

@gnought gnought changed the title Refactored Refactored Subscribe Handler Feb 7, 2020
@gnought
Copy link
Collaborator Author

gnought commented Feb 20, 2020

@mcollina does this PR look goods to you?

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gnought gnought merged commit 1c3aa35 into moscajs:master Feb 21, 2020
@gnought gnought deleted the feature/refactor branch February 21, 2020 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants