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

net,stream: remove DuplexBase #19779

Closed
wants to merge 2 commits into from
Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Apr 3, 2018

DuplexBase was added to prevent the "no-half-open enforcer" from
being inherited by net.Socket. The main reason to use it instead
of Duplex was that it allowed to not copy the options object but
since commit 5e3f516 the options object is copyed anyway so it is
no longer useful.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

`DuplexBase` was added to prevent the "no-half-open enforcer" from
being inherited by `net.Socket`. The main reason to use it instead
of `Duplex` was that it allowed to not copy the options object but
since commit 5e3f516 the options object is copyed anyway so it is
no longer useful.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels Apr 3, 2018
@lpinca
Copy link
Member Author

lpinca commented Apr 3, 2018

This basically reverts df07169.
CI: https://ci.nodejs.org/job/node-test-pull-request/14022/

util.inherits(Duplex, Readable);

{
// avoid scope creep, the keys array can then be collected
Copy link
Member

Choose a reason for hiding this comment

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

I don’t know about you, but “scope creep” means something pretty different for me… ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha.

lib/net.js Outdated
else
options = util._extend({}, options);

const aho = options.allowHalfOpen;
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit: I’d spell this out

@lpinca
Copy link
Member Author

lpinca commented Apr 6, 2018

@lpinca
Copy link
Member Author

lpinca commented Apr 6, 2018

Landed in 496d602.

@lpinca lpinca closed this Apr 6, 2018
@lpinca lpinca deleted the remove/duplex-base branch April 6, 2018 08:24
lpinca added a commit that referenced this pull request Apr 6, 2018
`DuplexBase` was added to prevent the "no-half-open enforcer" from
being inherited by `net.Socket`. The main reason to use it instead
of `Duplex` was that it allowed to not copy the options object but
since commit 5e3f516 the options object is copyed anyway so it is
no longer useful.

PR-URL: #19779
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2018
`DuplexBase` was added to prevent the "no-half-open enforcer" from
being inherited by `net.Socket`. The main reason to use it instead
of `Duplex` was that it allowed to not copy the options object but
since commit 5e3f516 the options object is copyed anyway so it is
no longer useful.

PR-URL: #19779
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants