Skip to content

net: remove Socket.prototoype.read - #18568

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:net-no-consuming
Closed

net: remove Socket.prototoype.read#18568
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:net-no-consuming

Conversation

@addaleax

@addaleaxaddaleax commented Feb 4, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Unused since 34b535f.

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

net

CI: https://ci.nodejs.org/job/node-test-commit/15943/

@addaleaxaddaleax added net Issues and PRs related to the net subsystem. lts-watch-v6.x labels Feb 4, 2018
@nodejs-github-botnodejs-github-bot added the net Issues and PRs related to the net subsystem. label Feb 4, 2018
@joyeecheung

joyeecheung commented Feb 5, 2018

Copy link
Copy Markdown
Member

CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1255/
CITGM against this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1256/
CI: https://ci.nodejs.org/job/node-test-pull-request/12937/

XadillaX
XadillaX approved these changes Feb 5, 2018
View reviewed changes

@XadillaXXadillaX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm.

@BridgeARBridgeAR added arm Issues and PRs related to the ARM architecture. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed arm Issues and PRs related to the ARM architecture. labels Feb 6, 2018
@apapirovski

apapirovski commented Feb 8, 2018

Copy link
Copy Markdown
Contributor

Landed in ef78a1e

apapirovski pushed a commit that referenced this pull request Feb 8, 2018
@addaleax@apapirovski
net: remove Socket.prototoype.read
ef78a1e
Unused since 34b535f.
PR-URL: #18568
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@addaleax@MylesBorins
net: remove Socket.prototoype.read
081c6ae
Unused since 34b535f.
PR-URL: #18568
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@addaleax@MylesBorins
net: remove Socket.prototoype.read
804393b
Unused since 34b535f.
PR-URL: #18568
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@addaleax@MylesBorins
net: remove Socket.prototoype.read
a5596b1
Unused since 34b535f.
PR-URL: #18568
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@addaleax@MylesBorins
net: remove Socket.prototoype.read
6ed4e69
Unused since 34b535f.
PR-URL: #18568
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@gibfahn

gibfahn commented Apr 13, 2018

Copy link
Copy Markdown
Member

Should this land on v6.x and v8.x? @addaleax I know you added the labels, just wanted to double-check as it seems (at first glance) that there's a possibility of this breaking someone who was expecting that function to be there.

@addaleax
addaleax deleted the net-no-consuming branch April 13, 2018 06:43
@addaleax

addaleax commented Apr 13, 2018

Copy link
Copy Markdown
MemberAuthor

@gibfahn The function is inherited from stream.Readable anyway, this was just a wrapper that we don’t need anymore on these versions

BethGriggs pushed a commit that referenced this pull request Apr 13, 2018
@addaleax@BethGriggs
net: remove Socket.prototoype.read
3225601
Unused since 34b535f.
PR-URL: #18568
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@addaleax@MayaLekova
net: remove Socket.prototoype.read
d44be40
Unused since 34b535f.
PR-URL: nodejs#18568
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+5 more reviewers
@apapirovskiapapirovskiapapirovski approved these changes
@danbevdanbevdanbev approved these changes
@evanlucasevanlucasevanlucas approved these changes
@XadillaXXadillaXXadillaX approved these changes
@hiroppyhiroppyhiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.netIssues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

13 participants

@addaleax@joyeecheung@apapirovski@gibfahn@danbev@jasnell@evanlucas@lpinca@XadillaX@hiroppy@BridgeAR@BethGriggs@nodejs-github-bot