Skip to content

Conversation

@turkyden
Copy link

@turkyden turkyden commented Apr 8, 2021

issue #5

Add The Typescript definitions. @gkubisa

@coveralls
Copy link

coveralls commented Apr 8, 2021

Coverage Status

Coverage remained the same at 98.969% when pulling 28ef3bd on Turkyden:patch-1 into 4a449d0 on Teamwork:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.969% when pulling f560b2f on Turkyden:patch-1 into 4a449d0 on Teamwork:master.

Copy link

@gkubisa gkubisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me so long to review it. Would you mind making some improvements before I merge this PR?

@@ -0,0 +1,16 @@
import { Duplex } from "stream";
import WebSocket from 'ws';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@types/ws would need to be added as a dependency to package.json.

declare class WebSocketJSONStream extends Duplex {
private _emittedClose;
private ws;
constructor(ws: WebSocket);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebSocketJSONStream works with a browser version of WebSocket too, so that should be taken into the account.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any ideas on how to address it? I think /// <reference lib="dom" /> should provide WebSocket... Would we then still need import WebSocket from 'ws';?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants