http: specify _implicitHeader in OutgoingMessage #7949
Conversation
|
I'd word as |
|
Fixed the wording and rebased :-) |
|
Is this necessary since |
|
Probably not. But I don't think it's a bad sanity check. |
|
No opinion on this change, but if we do land it, please add a test before landing, since there's nothing that will exercise this code otherwise. At a minimum, I'd think we could do something as simple as: assert.throws(http.OutgoingMessage.prototype._implicitHeader); |
|
Small nit: All the |
| var ServerResponse = http.ServerResponse; | ||
|
|
||
| assert.throws(OutgoingMessage.prototype._implicitHeader); | ||
| assert.ok(typeof ClientRequest.prototype._implicitHeader === 'function'); |
There was a problem hiding this comment.
Nit: Maybe use strictEqual() instead?:
assert.strictEqual(typeof ClientRequest.prototype._implicitHeader, 'function');There was a problem hiding this comment.
Fixed at 2c88993, thanks :-)
|
@Trott looks good to you now? If yes I will run a CI again, thank you :-) |
|
The test looks good to me. |
|
LGTM Thinking out loud: I've been wondering about the possibility of creating a new |
|
@jasnell IMO that's what an |
|
I think I'd be -1 to adding |
|
Perhaps we use |
|
I don't think asserting is the right thing to do here, as this is an unconditional error. |
|
Ok, I'm good with this as it is and definitely understand @cjihrig's argument :-) |
|
+1 on keeping |
|
Okay, and the CI seems to be green, how can we land this, should I land this by myself :-) |
|
Leave it open for another day so that it's open for at least 48 hours before landing. If no one has any objections after the PR has been open for 48 hours, then the two LGTMs you already have are enough and you can land it yourself at that time. |
PR-URL: nodejs#7949 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #7949 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
|
Landed at 99296ee, this is my first real contribution as a collaborator, so correct me if I'm wrong, thanks :-) |
|
The commit looks great @yorkie ! :-) |
PR-URL: #7949 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
|
want confirmation that this should be backported. it adds a new throw, but it would throw anyways correct? Just dug into the code and afaict the function seems unimplemented in OutgoingMessage or it's parent class Stream. |
|
@thealphanerd it should be implemented at https://github.com/nodejs/node/blob/master/lib/_http_client.js#L217-L220, its children class. |
|
@nodejs/lts should we backport this? |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
http
Description of change
I saw "lib/_http_outgoing.js" uses
_implicitHeaderfunction 3 times: line1, line2 and line3. But not searched this method defined atOutgoingMessage.prototype, found this method is defined at its children.