[connection-status] Use Websockets #2222
Conversation
|
I am doing the review now... Please note, your connection-status tests have failed on the CIs. |
|
|
||
| constructor( | ||
| @inject(ConnectionStatusOptions) @optional() protected readonly options: ConnectionStatusOptions = ConnectionStatusOptions.DEFAULT, | ||
| @inject(ILogger) protected readonly logger: ILogger | ||
| ) { | ||
| this.statusChangeEmitter = new Emitter<ConnectionStatus>(); |
There was a problem hiding this comment.
Can you please switch to property injection then?
There was a problem hiding this comment.
I want to use it without injection (see mock-connection-service.ts)
There was a problem hiding this comment.
I see, then it makes sense. 👍
|
|
||
| constructor( | ||
| @inject(WebSocketConnectionProvider) protected readonly wsConnectionProvider: WebSocketConnectionProvider, | ||
| @inject(PingService) protected readonly pingservice: PingService, |
There was a problem hiding this comment.
pingservice -> pingService please.
| this.fireStatusChange(newState); | ||
| } | ||
| // schedule offline | ||
| this.timer = this.setTimeout(async () => { |
|
|
||
| private scheduledPing: number | undefined; | ||
|
|
||
| constructor( |
There was a problem hiding this comment.
Please use property injection.
Edited: Obsolete. See: #2222 (comment)
|
|
||
| /** | ||
| * Clients can listen on connection status change events. | ||
| */ | ||
| readonly onStatusChange: Event<ConnectionStatus>; | ||
| readonly onStatusChange: Event<ConnectionState>; |
There was a problem hiding this comment.
It should be onStateChange.
There was a problem hiding this comment.
Also, updateStatus and fireStatusChange should be renamed to updateState and fireStateChange due to the ConnectionStatus removal.
There was a problem hiding this comment.
I changed state to status
Changed the connection status service to use websockets and natural activity. (fixes #1490) Signed-off-by: svenefftinge <sven.efftinge@typefox.io>
|
It works nicely. The tests are passing (locally) too. I have one more question; shouldn't we bind the |
There is no reason, it will be created once as it is requested only once. |
Alright, let's leave as is. |
Changed the connection status service to use websockets
and natural activity. (fixes #1490)