Skip to content

net: remove net.Socket.prototype.listen - #13735

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:deprecate-socket-listen
Closed

net: remove net.Socket.prototype.listen#13735
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:deprecate-socket-listen

Conversation

@BridgeAR

@BridgeARBridgeAR commented Jun 17, 2017•
edited
Loading

Copy link
Copy Markdown
Member

I suggest to deprecate this function. It was never documented even though it was already added in 2011. I am not sure what it was for and maybe @bnoordhuis might have a look as a original reviewer?
The function is defunct and shout simply be removed. As it was never documented there should not be a problem about it.

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

net

@nodejs-github-botnodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jun 17, 2017
@mscdexmscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 17, 2017
cjihrig
cjihrig reviewed Jun 19, 2017
View reviewed changes
Comment threadlib/net.js Outdated

@cjihrigcjihrigJun 19, 2017

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.

Can you remove the extra indentation, and change this line to:

}, 'Socket.prototype.listen() is deprecated.', 'DEP0073');

@jasnelljasnellJun 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The deprecation code should be DEP00XX... the actual value would be assigned when the PR lands

@jasnell

jasnell commented Jun 19, 2017

Copy link
Copy Markdown
Member

Ping @ChALkeR ... is it possible to get some idea of how much this is used?

@bnoordhuis

bnoordhuis commented Jun 20, 2017

Copy link
Copy Markdown
Member

Not opposed but it's neither broken nor a maintenance hassle, just a wart.

@BridgeAR

BridgeAR commented Jun 20, 2017

Copy link
Copy Markdown
MemberAuthor

@bnoordhuis would you recommend to document this part instead of deprecating it? Because I think the current situation should be changed one way or the other.

@BridgeAR

BridgeAR commented Jun 23, 2017

Copy link
Copy Markdown
MemberAuthor

PTAL

@cjihrig

cjihrig commented Jun 23, 2017

Copy link
Copy Markdown
Contributor

@BridgeARhttps://github.com/nodejs/node/pull/13735/files#r122780215 still applies I think.

@ChALkeRChALkeR self-assigned this Jun 24, 2017
@BridgeAR

BridgeAR commented Jun 26, 2017

Copy link
Copy Markdown
MemberAuthor

@cjihrig sorry, I didn't address the comment right away because I thought there might be feedback to my question. I fixed that now though.

jasnell
jasnell reviewed Jun 26, 2017
View reviewed changes
Comment threaddoc/api/deprecations.md Outdated

@jasnelljasnellJun 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The deprecation code here should be DEP00XX. The actual number should be assigned when the PR lands.

@BridgeAR
BridgeARforce-pushed the deprecate-socket-listen branch from 9388c56 to 3929c94CompareJune 26, 2017 21:09
@BridgeAR

BridgeAR commented Jun 26, 2017

Copy link
Copy Markdown
MemberAuthor

Comment addressed and rebased

@BridgeAR

BridgeAR commented Aug 25, 2017

Copy link
Copy Markdown
MemberAuthor

PTAL - we might also still just go ahead and document this instead of deprecating it.
(I will rebase as soon as there is a conclusion)

@jasnell

jasnell commented Aug 25, 2017

Copy link
Copy Markdown
Member

@BridgeAR ... that would be my preference.

@BridgeAR

BridgeAR commented Dec 6, 2017

Copy link
Copy Markdown
MemberAuthor

I am closing this as I do not really know how to document this as I do not know what it should be used for.

If someone knows more, please let me know and I will have a look at it again.

@BridgeARBridgeAR closed this Dec 6, 2017
@bnoordhuis

bnoordhuis commented Dec 6, 2017

Copy link
Copy Markdown
Member

@BridgeAR The idea is that net.Socket#listen() enables process.stdin.listen(), which at one time was necessary to make FastCGI work.

Looking at the code again, I'm pretty sure it's broken (as in: unusable, throws TypeErrors) and has been for a long time so there is no point in deprecating it. It can be removed outright.

@BridgeAR

BridgeAR commented Dec 6, 2017

Copy link
Copy Markdown
MemberAuthor

@bnoordhuis thanks a lot for the info! And yes, I just checked and the function would simple throw a TypeError when called. Removing seems to be the right thing to do here.

@BridgeARBridgeAR reopened this Dec 6, 2017
@BridgeAR
BridgeARforce-pushed the deprecate-socket-listen branch from 3929c94 to 12118fdCompareDecember 6, 2017 16:52
@BridgeAR
net: remove Socket.prototype.listen
b2fc8da
The function was never documented and now throws a TypeError if used.
@BridgeAR
BridgeARforce-pushed the deprecate-socket-listen branch from 12118fd to b2fc8daCompareDecember 6, 2017 16:53
@BridgeARBridgeAR changed the title net: deprecate net.Socket.prototype.listennet: remove net.Socket.prototype.listenDec 6, 2017
@apapirovski

apapirovski commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/12009/

I've also removed the semver-major label as this method doesn't even work.

@apapirovskiapapirovski removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 9, 2017
@apapirovskiapapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 9, 2017
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 12, 2017
@BridgeAR
net: remove Socket.prototype.listen
e17dba7
The function was never documented and now throws a TypeError if used.
PR-URL: nodejs#13735
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@BridgeAR

BridgeAR commented Dec 12, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in e17dba7

@addaleaxaddaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
MylesBorins pushed a commit that referenced this pull request Dec 20, 2017
@BridgeAR@MylesBorins
net: remove Socket.prototype.listen
4a2bd10
The function was never documented and now throws a TypeError if used.
PR-URL: #13735
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2017
@BridgeAR@MylesBorins
net: remove Socket.prototype.listen
e58a5ca
The function was never documented and now throws a TypeError if used.
PR-URL: #13735
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorinsMylesBorins mentioned this pull request Jan 10, 2018
Merged
MylesBorins added a commit that referenced this pull request Jan 10, 2018
@MylesBorins
2019-01-10 Version 9.4.0 (Current)
074a2cb
Notable change:
* async_hooks:
- deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
API were documented. (Andreas Madsen)
#16972
* deps:
- update nghttp2 to 1.29.0 (James M Snell)
#17908
- upgrade npm to 5.6.0 (Kat Marchán)
#17535
- cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
#16591
* events:
- remove reaches into _events internals (Anatoli Papirovski)
#17440
* http:
- add rawPacket in err of `clientError` event (XadillaX)
#17672
* http2:
- implement maxSessionMemory (James M Snell)
#17967
- add initial support for originSet (James M Snell)
#17935
- add altsvc support (James M Snell)
#17917
- perf_hooks integration (James M Snell)
#17906
* net:
- remove Socket.prototype.write (Anna Henningsen)
#17644
- remove Socket.prototype.listen (Ruben Bridgewater)
#13735
* repl:
- show lexically scoped vars in tab completion (Michaël Zasso)
#16591
* stream:
- rm {writeable/readable}State.length (Calvin Metcalf)
#12857
- add flow and buffer properties to streams (Calvin Metcalf)
#12855
* util:
- allow wildcards in NODE_DEBUG variable (Tyler)
#17609
* zlib:
- add ArrayBuffer support (Jem Bezooyen)
#16042
* Addedew collaborator**
- [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
- [danbev](https://github.com/danbev) Daniel Bevenius
PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
@MylesBorins
2019-01-10 Version 9.4.0 (Current)
7aefa40
Notable change:
* async_hooks:
- deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
API were documented. (Andreas Madsen)
#16972
* deps:
- update nghttp2 to 1.29.0 (James M Snell)
#17908
- upgrade npm to 5.6.0 (Kat Marchán)
#17535
- cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
#16591
* events:
- remove reaches into _events internals (Anatoli Papirovski)
#17440
* http:
- add rawPacket in err of `clientError` event (XadillaX)
#17672
* http2:
- implement maxSessionMemory (James M Snell)
#17967
- add initial support for originSet (James M Snell)
#17935
- add altsvc support (James M Snell)
#17917
- perf_hooks integration (James M Snell)
#17906
- Refactoring and cleanup of Http2Session and Http2Stream destroy
(James M Snell) #17406
* net:
- remove Socket.prototype.write (Anna Henningsen)
#17644
- remove Socket.prototype.listen (Ruben Bridgewater)
#13735
* repl:
- show lexically scoped vars in tab completion (Michaël Zasso)
#16591
* stream:
- rm {writeable/readable}State.length (Calvin Metcalf)
#12857
- add flow and buffer properties to streams (Calvin Metcalf)
#12855
* util:
- allow wildcards in NODE_DEBUG variable (Tyler)
#17609
* zlib:
- add ArrayBuffer support (Jem Bezooyen)
#16042
* Addedew collaborator**
- [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
- [danbev](https://github.com/danbev) Daniel Bevenius
PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
@MylesBorins
2019-01-10 Version 9.4.0 (Current)
e688175
Notable change:
* async_hooks:
- deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
API were documented. (Andreas Madsen)
#16972
* deps:
- update nghttp2 to 1.29.0 (James M Snell)
#17908
- upgrade npm to 5.6.0 (Kat Marchán)
#17535
- cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
#16591
* events:
- remove reaches into _events internals (Anatoli Papirovski)
#17440
* http:
- add rawPacket in err of `clientError` event (XadillaX)
#17672
* http2:
- implement maxSessionMemory (James M Snell)
#17967
- add initial support for originSet (James M Snell)
#17935
- add altsvc support (James M Snell)
#17917
- perf_hooks integration (James M Snell)
#17906
- Refactoring and cleanup of Http2Session and Http2Stream destroy
(James M Snell) #17406
* net:
- remove Socket.prototype.write (Anna Henningsen)
#17644
- remove Socket.prototype.listen (Ruben Bridgewater)
#13735
* repl:
- show lexically scoped vars in tab completion (Michaël Zasso)
#16591
* stream:
- rm {writeable/readable}State.length (Calvin Metcalf)
#12857
- add flow and buffer properties to streams (Calvin Metcalf)
#12855
* util:
- allow wildcards in NODE_DEBUG variable (Tyler)
#17609
* zlib:
- add ArrayBuffer support (Jem Bezooyen)
#16042
* Addedew collaborator**
- [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
- [danbev](https://github.com/danbev) Daniel Bevenius
PR-URL: #18069
@MylesBorins

MylesBorins commented Jan 22, 2018

Copy link
Copy Markdown
Contributor

Didn't land on v6.x, should it be considered for v8.x?

@MylesBorins

MylesBorins commented May 22, 2018

Copy link
Copy Markdown
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@codebyterecodebytere mentioned this pull request Jun 6, 2018
Closed
3 tasks
@BridgeAR
BridgeAR deleted the deprecate-socket-listen branch April 1, 2019 23:37
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell left review comments
@bnoordhuisbnoordhuisbnoordhuis approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@apapirovskiapapirovskiapapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

netIssues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@BridgeAR@jasnell@bnoordhuis@cjihrig@apapirovski@MylesBorins@mscdex@ChALkeR@addaleax@TimothyGu@nodejs-github-bot