-
-
Notifications
You must be signed in to change notification settings - Fork 151
feat: support watch protocol v1 #2411
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
base: main
Are you sure you want to change the base?
Conversation
|
| */ | ||
| async awaitFirstCycle() { | ||
| if (this._version > 0) { | ||
| await this._send<SubscribeMessage>(MessageType.SUBSCRIBE, { |
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.
Maybe this is just re-inventing the CAP message from the original proposal?
@thesayyn were you thinking that a socket has a single subscription, and the CAP message is essentially declaring the options for that socket/subscription? This PR does similar but "subscribe" seems to imply that you can have more then one subscription per socket - which is probably more complicated and not worth it?
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.
my worry with SUBSCRIBE is that it blocks cycle messages from being sent until the implementor tells us what it needs. the name implies that it can be sent N many times and every subscription needs an id to distinguish the CYCLE events from each other.
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.
I'm starting to think we should keep the existing behaviour where 1 socket = 1 subscription, and this PR (and the associated protocol spec changes) should instead implement the original CAP message - which I think aligns with your original idea?
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.
Yes, and revisit the scope idea where as part of the CAP, we can decide what the user is asking for.




Changes are visible to end-users: yes/no
Test plan