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.

Sign up for GitHub

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

Jump to bottom

[v8.x backport] net: remove Socket.prototype.listen #21176

Closed
wants to merge 1 commit into from
Closed

[v8.x backport] net: remove Socket.prototype.listen #21176

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jun 6, 2018 •
edited
Loading

Original PR: #13735

Checklist
  • make -j4 test passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@BridgeAR @codebytere
net: remove Socket.prototype.listen
94bcdc9 
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>
@nodejs-github-bot nodejs-github-bot added net Issues and PRs related to the net subsystem. v8.x labels Jun 6, 2018
@ryzokuken
Copy link
Contributor

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

@ryzokuken ryzokuken added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 10, 2018
@ryzokuken
Copy link
Contributor

Okay, tests finally passed.

@MylesBorins shall we land this?

@MylesBorins
Copy link
Contributor

My only concern with landing this is that it has the risk of breaking something on 8.x that we don't know about. I'm not sure this actually makes sense for LTS at the moment without someone else chiming in... specifically because we don't really have anything huge to gain by landing it, but if we have even a 1% risk of breaking things we likely shouldn't.

Perhaps we should run citgm?

@ryzokuken
Copy link
Contributor

@MylesBorins it's an undocumented and defunct method, and I hope nobody from the ecosystem is using it, but yeah, why not run CITGM and know for sure?

@joyeecheung
Copy link
Member

Landing on v8.x this seems to break our LTS policy..this has been released in v9.x but has never been tested in older release lines.

@codebytere
Copy link
Member Author

@joyeecheung is there a way i can tell if that's the case from looking at PRs which have the backport-requested label? At present i was just looking through the lists from LTS and current for closed PRs which have the label but i'd like to ensure i'm doing work that helps yall and doesn't break policies!

@TimothyGu
Copy link
Member

@codebytere Unfortunately, we are behind on triaging pull requests for backporting… I’ve manually unmarked the original PR for backporting, but I don’t really have a good way for you tell which ones should be backported, other than comparing a change to our LTS policies (no potential breaking changes of any kind, unless the API is experimental). Sorry about that.

@TimothyGu TimothyGu added blocked PRs that are blocked by other issues or PRs. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 11, 2018
@codebytere
Copy link
Member Author

codebytere commented Jun 11, 2018 •
edited
Loading

@TimothyGu no worries, thanks! Should this backport be closed?

@ryzokuken
Copy link
Contributor

ryzokuken commented Jun 11, 2018 •
edited
Loading

@codebytere yes, please close this if it's okay.

@codebytere codebytere closed this Jun 12, 2018
@codebytere codebytere deleted the backport-13735-to-v8.x branch June 12, 2018 15:46
@joyeecheung
Copy link
Member

@codebytere What @TimothyGu said, sorry about the confusion. We probably should've marked that PR dont-land-on-v8.x before it lands, there are no clear documentation on how to triage PRs like that but in general, removing methods/properties, even the undocumented ones, should be treated with care.

@codebytere
Copy link
Member Author

thanks @joyeecheung, appreciate the context!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@codebytere @ryzokuken @MylesBorins @joyeecheung @TimothyGu @lpinca @nodejs-github-bot @BridgeAR