Closed roddolf closed 6 years ago
@noelblaschke @mitkpl @JimiC as you are all concerned by the index.ts, are you ok with that ?
I'm currently out of town. Will get back to you on Monday. From a sneak peek I had at it, I may have some objections.
As I'm back earlier, I did the review and expressed my proposed changes.
Added changes for the Client
class.
In the case of the Frame
class, think this should be left generic since it can receive any of the headers' interfaces already declared and more.
So I returned the ExtendedHeaders
to the Message
interface.
Also changed the disconnectCallback
as optional since the code allow it.
@roddolf
disconnectCallback
is optional.Headers
(I guess the ExtendedHeaders
of Message
will override the Headers
of Frame
). And while we are doing changes, if I recall correct new tslint
rules dictate that Array<string>
types should be declared as string[]
. So, if it's not much to ask could you change that as well?
OK. Last thing. As discussed here we should also expose the debug
function.
So could you also add debug(...args: any[]): void;
in Client
?
And your name and email in "// Definitions by: " section. You deserve some credits.
Ready, thanks @JimiC
Just, if the other pull request is merged, the definitions of the marshall
method must be updated too, right?
Yes. But the other PR is debatable.
@JSteunou We are done here. Sorry for making your life miserable today. The fact though that other devs are iterating over the types is an indication that your library is gaining popularity among the typescript
community. Joy.
I update the typings to include the properties of a Frame, and Message to inherit these properties.
I also expose the
connected
flag in the Client class. I don't know if the other properties are meant to be private, but at least I think this property is useful, so that is the only one I added.