doc: update http.md mention of socket #30155
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit is addressing the problem in issue nodejs#29948
nodejs-github-bot
added
doc
|
Trying to see if this is part of the solution suggested in #29948. If it is, I also modify the other mentions of |
Updating the docs based on feedback from @addaleax Fixes: nodejs#29948
addaleax
reviewed
Oct 30, 2019
doc/api/http.md
Outdated
| Emitted after a socket is assigned to this request. | ||
| Emitted after a socket is assigned to this request. This event may be passed | ||
| a socket {net.Socket} object, an extendion the duplex object, however this | ||
| behavior is not guaranteed. |
There was a problem hiding this comment.
Two things:
an extendion the duplex object→ soething likea subclass of `Duplex`- The behaviour is guaranteed if the user didn’t specifically take action to provide a socket that is not a
net.Socketinstance
There was a problem hiding this comment.
Roger that, makes complete sense. I have updated this PR @addaleax to match both of your suggestions.
jessekoconnor
force-pushed
the
patch-1
branch
from
96e4f79
to
863f6f4
Compare
|
Quick silly question for clarification @addaleax: Thanks in advance for clarification. Having a good time digging deep into the http module :D |
|
This is currently how the docs look when running this locally just now (with all 4 commits): |
addaleax
approved these changes
Oct 30, 2019
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Errm, tbh I’ve never thought about The main way to make this scenario happen on the client side would be to pass an asynchronous function as the being createConnection: common.mustCall((options, cb) => {
setImmediate(() => { // or some other async thing
cb(null, clientSide);
});
})instead. |
|
Ended up making changes only to the events. If we want to make changes to the methods, thats an option but only if we want to encourage users to use duplex instead of socket, and I don't think we want that. Anyways, I am going to do another checkover in the morning but this may be close to completion. |
addaleax
approved these changes
Oct 31, 2019
We want them to be able to do that, and encourage it when it fits their use case, but what we should avoid imo is to present non- |
@addaleax, added a commit that suggests a line to add to relevant methods that doesn't imply duplex usage as preferred: 5b827a3 |
tniessen
approved these changes
Nov 1, 2019
Co-Authored-By: Tobias Nießen <tniessen@users.noreply.github.com>
Co-Authored-By: Tobias Nießen <tniessen@users.noreply.github.com>
Co-Authored-By: Tobias Nießen <tniessen@users.noreply.github.com>
|
Gah, I just messed up.. Heres what I did in my attempt to rebase according to the contribution docs in case anyone has any suggestions on how I can fix this: |
|
Starting to think I should just create a new branch from master and chery-pick in all of my commits in the proper order. Any thoughts on this are greatly appreciated, thanks in advance |
jessekoconnor
force-pushed
the
patch-1
branch
from
37b3cdb
to
1fc5adf
Compare
|
Heck yeah - managed to unfudge that bad rebase.. Woot woot! Ready for review!! |
|
Latest local test run: |
addaleax
approved these changes
Nov 6, 2019
1 similar comment
addaleax
added
the
author ready
|
@addaleax - silly question; Is there an action for me to complete on that ci link you posted above? or is that just for ease of future review Thanks in advance 😊 |
|
@jessekoconnor No, generally not – as long as CI is green or there are no related failures, all should be good 👍 |
gireeshpunathil
approved these changes
Nov 8, 2019
|
@mcollina @tniessen - PTAL. (looks like @jessekoconnor 's review request invalidated your earlier approval status - as seen in the review summary in the browser) |
jasnell
approved these changes
Nov 12, 2019
|
@tniessen - Please review when you find a minute - my re-review request invalidated your original approval. Sorry for the inconvenience. |
addaleax
pushed a commit
that referenced
this pull request
Nov 19, 2019
This commit is addressing the problem in issue #29948. Fixes: #29948 PR-URL: #30155 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Finally :) Landed in 7cecac6, thanks for the contribution! 🎉 |
|
Thanks everybody, feels really good to get that merged. I’ll be looking myself, but If anyone sees additional work suitable for me, I welcome an @ mention 😄 |
MylesBorins
pushed a commit
that referenced
this pull request
Nov 21, 2019
This commit is addressing the problem in issue #29948. Fixes: #29948 PR-URL: #30155 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
targos
pushed a commit
that referenced
this pull request
Jan 13, 2020
This commit is addressing the problem in issue #29948. Fixes: #29948 PR-URL: #30155 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs
pushed a commit
that referenced
this pull request
Feb 6, 2020
This commit is addressing the problem in issue #29948. Fixes: #29948 PR-URL: #30155 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit is addressing the problem in issue #29948
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes