-
Notifications
You must be signed in to change notification settings - Fork 16
Support push #62
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?
Support push #62
Conversation
1459aa2 to
74e5584
Compare
| Push (web sockets) usage example for mailboxes | ||
| ```typescript | ||
| const mailboxObservable = client.pushMailbox().subscribe(); | ||
| client.pushStart(); |
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 I configure the JMAP types I want to receive push updates for? By passing a list of types when starting the push?
Registrating all type at once rather than one by one can be cool IMO.
Also an optional debounce at the transport level might be very handy (Eg Ensuring an initial 1 s delay before the first notification and no more than one notification every 2 seconds). Note that several StateChanges can very easily be collapsed into one.
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.
If you
client.pushMailbox().subscribe();
client.pushEmailSubmission().subscribe();
client.pushEmail().subscribe();
client.pushStart();
Every types will be registered at once when pushStart is called.
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.
Also an optional debounce at the transport level might be very handy (Eg Ensuring an initial 1 s delay before the first notification and no more than one notification every 2 seconds). Note that several StateChanges can very easily be collapsed into one.
If JMAP can offer a debounce parameter, why not use it yes. I don't think setting a debounce on the client is really useful though.
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.
Every types will be registered at once when pushStart is called.
Nice, so it's doing the job.
IMO maybe the API can be refined to be friendlier but I let that to your jugement.
If JMAP can offer a debounce parameter, why not use it yes.
It does not.
I don't think setting a debounce on the client is really useful though.
I think it is useful to ensure a single client does not generate too much traffic. We then get an easy model to think about the amount of load generated by a single client.
Related to linagora#33
24fa11f to
81f2091
Compare
|
Rebased on main |
|
I think getApiUrl, getPushUrl, overriddenPushUrl may be useless. |
I will make another PR to remove completely the getPushUrl & getApiUrl and the overridden*Url params. |
| Push (web sockets) usage example for mailboxes | ||
| ```typescript | ||
| const mailboxObservable = client.pushMailbox().subscribe(); | ||
| client.pushStart(); |
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.
Subscribe and start are similar in semantic. I d expect only one.
Where is the callback?
Also it would imo be nice to support push notifications for arbitrary types...
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.
Subscribe and start are similar in semantic. I d expect only one.
Calling pushStart then subscribe is not the same than calling subscribe then pushStart.
We can talk about a way you would like to call methods, a scenario, and what you expect to happen under the hood.
Where is the callback?
You can use a callback as parameter of the subscribe method, or you can do what you want with the mailboxObservable. See https://rxjs.dev/guide/overview
Also it would imo be nice to support push notifications for arbitrary types...
What would be the use case?
| value: string; | ||
| }>(`${this.client.getApiUrl()}/ws/ticket`, '', this.httpHeaders) | ||
| .then(response => { | ||
| const ticket = response.value; |
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.
Im not sure this non standard auth sheme should ve hard coded as the default.
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.
There is currently no alternative as far as I know.
I would be glad to add another way to open a websocket.
I also replaced the hard-coded url by the capability one.
| return this.pushForEntityType('EmailSubmission'); | ||
| } | ||
|
|
||
| private pushForEntityType(entityType: IEntityType): Observable<{ [accountId: string]: string }> { |
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 allow passing an array
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.
Since this method is private, you do not get to use it anyway.
If needed by the calling methods, the method will be modified later.
chibenwa
left a comment
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.
In vacation, reviewed from my phone...
Related to #33