-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
@mkg20001 would love to have your review on this PR so that you could check and see what I refactored. Overall there is a lot of work here! Thank you so much for pushing this 👏🏽👏🏽 I improved the code a bit just by following the standard style we use for our JS projects. One thing that I caught often was One of the ways to ensure that your code is in check before committing is by using the Standard code style linter, it is "code style" but it captures some bugs such as things undefined, different name, errors not handled and so on. You can check it here https://standardjs.com/, there are multiple plugins and there should be one for the editor you use. In last resort, you can always run There is still more parts of the code that can be modularized and tested so that it gets easier to maintain. Nevertheless, it is super cool to see so many tests here and in the transport module 💯 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great.
Awesome work you have done! 👍
This will install a `websockets-star` CLI tool. Now you can spawn the server with: | ||
|
||
```bash | ||
> websockets-star --port=9090 --host=127.0.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there an s? It's still named websocket-star
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a typo. I fixed it a bunch of times myself and still missed this one, my bad!
I just moved things around, you are the one who crushed it! 👏🏽👏🏽👏🏽 |
Following libp2p/js-libp2p-websocket-star#16 (comment)