Prevents infinite WebSocket reconnection spam on subsequent token expiration signals #127
Conversation
cwhitten
requested review from
compulim,
ckkashyap,
sgellock and
dandriscoll
| connectionStatusFrom: ConnectionStatus, | ||
| connectionStatusTo: ConnectionStatus, | ||
| maxAttempts = 5 | ||
| ) { |
There was a problem hiding this comment.
Indentation.
| ) { | |
| ) { |
| return (status: ConnectionStatus): ConnectionStatus => { | ||
| if (status === connectionStatusFrom && currStatus === status) { | ||
| if (attempts >= maxAttempts - 1) { | ||
| attempts = 0 |
There was a problem hiding this comment.
Semi-colon.
| attempts = 0 | |
| attempts = 0; |
| let attempts = 0; | ||
| let currStatus = null; | ||
| return (status: ConnectionStatus): ConnectionStatus => { | ||
| if (status === connectionStatusFrom && currStatus === status) { |
There was a problem hiding this comment.
How about doing two-if on one line?
| if (status === connectionStatusFrom && currStatus === status) { | |
| if (status === connectionStatusFrom && currStatus === status && attempts >= maxAttempts - 1) { |
| connectionStatusTo: ConnectionStatus, | ||
| maxAttempts = 5 | ||
| ) { | ||
| let attempts = 0; |
There was a problem hiding this comment.
If we initially set attempts = 1, then we can get rid of the maxAttempts - 1 below.
|
|
||
| test('#setConnectionStatusFallback', () => { | ||
| const { DirectLine } = DirectLineExport; | ||
| expect(typeof DirectLine.prototype.setConnectionStatusFallback).toBe('function') |
There was a problem hiding this comment.
How about moving this line below so we can do
| expect(typeof DirectLine.prototype.setConnectionStatusFallback).toBe('function') | |
| expect(typeof setConnectionStatusFallback).toBe('function'); |
| test('#setConnectionStatusFallback', () => { | ||
| const { DirectLine } = DirectLineExport; | ||
| expect(typeof DirectLine.prototype.setConnectionStatusFallback).toBe('function') | ||
| const { setConnectionStatusFallback } = DirectLine.prototype |
There was a problem hiding this comment.
Semi-colon (and below).
| const { setConnectionStatusFallback } = DirectLine.prototype | |
| const { setConnectionStatusFallback } = DirectLine.prototype; |
| const { setConnectionStatusFallback } = DirectLine.prototype | ||
| const testFallback = setConnectionStatusFallback(0, 1) | ||
| let idx = 4 | ||
| while(idx--) { |
There was a problem hiding this comment.
Space before parenthesis.
| while(idx--) { | |
| while (idx--) { |
| // fallback will be triggered | ||
| expect(testFallback(0)).toBe(1) | ||
| idx = 4 | ||
| while(idx--) { |
There was a problem hiding this comment.
| while(idx--) { | |
| while (idx--) { |
| "json", | ||
| "node" | ||
| ], | ||
| } |
There was a problem hiding this comment.
New empty line before EOF.
|
FYI, me and @ckkashyap were building the test harness (integration tests) here, https://github.com/compulim/BotFramework-DirectLineJS/blob/preview/packages/test-harness/src/createServer.test.ts. We aren't moving it in yet, so I am good for the current work. |
| "jsx", | ||
| "json", | ||
| "node" | ||
| ], |
There was a problem hiding this comment.
Remove comma.
| ], | |
| ] |
| "js", | ||
| "jsx", | ||
| "json", | ||
| "node" |
There was a problem hiding this comment.
.node seems less used, but I am fine to keep it here.
…iration signals (#127) * Prevents infinite WebSocket reconnection spam on subsequent token expiration signals
Currently when a token expires and the browser is using WebSockets, this client library will infinitely attempt to reconnect in very fast iterations to the DirectLine server and receive 403's forever.
A couple things introduced here to address this issue: