-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
quic: remove noop code #33914
quic: remove noop code #33914
Conversation
this.unidirectional depends on #id which is never set in the constructor, hence this condition will never run and can be removed.
|
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.
Yep, I believe this is a remnant from a prior refactoring that changed when the #id
was set so we should be safe in removing this. FWIW, after we get past the openjs world conference and collaborator summit next week, I plan on revisiting all of the JavaScript code in here and doing a major refactoring. Obviously, feel free to keep making PRs changing bits but there a large portion of this that I fully expect to see changed.
Make sure you |
Cool. I'll make some draft PR's as some kind of post merge review, against my repo and ping you. At least on the streams part I would like to give some feedback on what I'd propose to change/improve. Please give me a shout out once you are past your refactoring and I'll do dig in. |
@jasnell Does CI run with --experimental-quic? |
It does not. You could always run |
See also: nodejs/build#2353 |
this.unidirectional depends on #id which is never set in the constructor, hence this condition will never run and can be removed. PR-URL: #33914 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Landed in 16116f5 |
this.unidirectional
depends on#id
which is never set in the constructor, hence this condition will never run and should be possible to removed.I'm probably missing something here? I guess this might be more of a question in the form of a PR. However, the tests seem to pass.
My original intent was to try and remove the explicit push/end and instead send in
readable
/writable
to theDuplex
.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes