Review comments #84
Review comments #84
Conversation
src/stream.ts
Outdated
| this.channel.send(sendbuf.subarray()) | ||
| } | ||
|
|
||
| // Close the channel after the source has ended |
There was a problem hiding this comment.
This tripped me up as well. See libp2p/js-libp2p-webtransport#23
There was a problem hiding this comment.
The referenced PR and the current implementation would both break on cases similar to:
const response = await pipe([request], stream, sink)
// process response
await pipe([request1], stream)
However, it might be better to use it-pushable or it-handshake in these cases.
I've refactored the sink to be more readable, and also limited sink to being called once similar to the referenced PR.
There was a problem hiding this comment.
The referenced PR and the current implementation would both break on cases similar to...
Yes. That's intentional in the js-libp2p design. And the workaround is using a pushable like you mentioned. I'm not a fan of this, but this is how everything else is setup, so it would be even more confusing if this deviated from it.
|
|
||
| // no default | ||
| default: | ||
| unreachableBranch(flag) |
| } | ||
|
|
||
| if (currentState === StreamStates.OPEN || currentState === StreamStates.WRITE_CLOSED) { | ||
| this._sendFlag(pb.Message_Flag.STOP_SENDING) |
There was a problem hiding this comment.
Interesting, I wonder how that semicolon got past the linter 🤔
|
Thanks for the PR @MarcoPolo! The refactors make the code more readable 💯. @ckousik will respond to 2 of the TODOs on Monday. |
|
@MarcoPolo I've addressed the changes in https://github.com/libp2p/js-libp2p-webrtc/tree/ckousik/review . Can I push to the branch on this PR instead? |
|
@ckousik please do. Edit, I already did it. |
| */ | ||
| reset (): void { | ||
| // TODO Why are you resetting the stat here? | ||
| this.stat = defaultStat(this.stat.direction) |
There was a problem hiding this comment.
I think we should remove this. Am I missing something?
|
Merging this as my repo review comments. @achingbrain this repo is ready for your review whenever you're ready. |
|
🎉 This PR is included in version 1.0.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Overall this looks good. I added some changes and left a couple of questions in the code. Once those questions are addressed and my changes make sense, I can update the code and merge these changes.
I ran eslint on the files I touched. It may be better to review with the
?w=1option to ignore whitespace changes.