Fix connection.test.js #211
Conversation
timoxley
left a comment
timoxley
left a comment
There was a problem hiding this comment.
Connection debug parameter should be optional but otherwise lgtm.
| isWaiting?: Todo | ||
| _isReconnecting: Todo | ||
| _backoffTimeout: Todo | ||
| sendID: Todo |
There was a problem hiding this comment.
Did you try removing :Todo, some of these might "just work" as of TS 4.0: https://devblogs.microsoft.com/typescript/announcing-typescript-4-0/#class-property-inference
There was a problem hiding this comment.
Yes, onReconnecting and onMessage work also ok without type definition. Marked the fields as Todo so that we'll define the actual type in the future (and the full benefit of the type system).
src/Connection.ts
Outdated
| } | ||
|
|
||
| constructor(options = {}, client) { | ||
| constructor(options = {}, debug: Debug.Debugger) { |
There was a problem hiding this comment.
Note original code worked like this:
f3bae13#diff-0e149db9f5c84dd5e3126b72e3cdcf3c297e6f2836d6a1b6bf0dea41cf05760fL66-L71
if (options.debug) {
this._debug = options.debug.extend(id)
} else {
this._debug = Debug(`StreamrClient::${id}`)
}debug should be optional.
There was a problem hiding this comment.
Changed that to an optional parameter.
In the previous PR I marked debug as a required parameter because I looked like the Connection and StreamrClient debug instances are in practice linked together (in production code Connection's debug was always derived from StreamrClient's debug). But maybe optionality better as it makes Connection more independent in theory.
No description provided.