Skip to content
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

Closed
wants to merge 1 commit into from
Closed

quic: remove noop code #33914

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 16, 2020

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 the Duplex.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

this.unidirectional depends on #id which is never set
in the constructor, hence this condition will never run
and can be removed.
@ronag ronag added the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Jun 16, 2020
@ronag ronag requested a review from jasnell June 16, 2020 20:38
@ronag
Copy link
Member Author

ronag commented Jun 16, 2020

make -j4 && python tools/test.py -J --mode=release parallel/test-quic-* is that enough to run quic related tests?

Copy link
Member

@jasnell jasnell left a 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.

@jasnell
Copy link
Member

jasnell commented Jun 16, 2020

make -j4 && python tools/test.py -J --mode=release parallel/test-quic-* is that enough to run quic related tests?

Make sure you ./configure --experimental-quic first, then yes, that works for running the necessary tests.

@ronag
Copy link
Member Author

ronag commented Jun 16, 2020

I plan on revisiting all of the JavaScript code in here and doing a major refactoring

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.

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Jun 16, 2020

@jasnell Does CI run with --experimental-quic?

@richardlau
Copy link
Member

Does CI run with --experimental-quic?

It does not. You could always run node-test-commit or one of the more platform specific node-test-commit-* jobs (i.e. not the node-test-pull-request job) with a custom CONFIG_FLAGS build parameter.

@jasnell
Copy link
Member

jasnell commented Jun 16, 2020

See also: nodejs/build#2353

jasnell pushed a commit that referenced this pull request Jun 18, 2020
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>
@jasnell
Copy link
Member

jasnell commented Jun 18, 2020

Landed in 16116f5

@jasnell jasnell closed this Jun 18, 2020
@jasnell jasnell added the experimental Issues and PRs related to experimental features. label Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Issues and PRs related to experimental features. quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants