http: document that ClientRequest inherits from OutgoingMessage

[kcak11]

No description provided.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[mcollina]

Why are you suggesting this change? The previous examples looked correct to me.

[kcak11]

Why are you suggesting this change? The previous examples looked correct to me.

The previous examples used request.headers but request.headers is no longer enumerable and we need to access the headers using the accessor request.getHeaders(). Trying to use request.headers results in undefined.

Refer: https://nodejs.org/api/http.html#messageheaders

[mcollina]

message.headers is defined as

node/lib/_http_incoming.js

Lines 107 to 124 in 2468db1

	ObjectDefineProperty(IncomingMessage.prototype, 'headers', {
	get: function() {
	if (!this[kHeaders]) {
	this[kHeaders] = {};
	const src = this.rawHeaders;
	const dst = this[kHeaders];
	for (let n = 0; n < this[kHeadersCount]; n += 2) {
	this._addHeaderLine(src[n + 0], src[n + 1], dst);
	}
	}
	return this[kHeaders];
	},
	set: function(val) {
	this[kHeaders] = val;
	}
	});

. There is no getHeaders() in IncomingMessage.

[kcak11]

message.headers is defined as

node/lib/_http_incoming.js

Lines 107 to 124 in 2468db1

	ObjectDefineProperty(IncomingMessage.prototype, 'headers', {
	get: function() {
	if (!this[kHeaders]) {
	this[kHeaders] = {};
	const src = this.rawHeaders;
	const dst = this[kHeaders];
	for (let n = 0; n < this[kHeadersCount]; n += 2) {
	this._addHeaderLine(src[n + 0], src[n + 1], dst);
	}
	}
	return this[kHeaders];
	},
	set: function(val) {
	this[kHeaders] = val;
	}
	});

. There is no getHeaders() in IncomingMessage.

Please see the below code snippet:

var http = require('http');
var options = {
    'method': 'GET',
    'hostname': 'www.example.com',
    'path': '/',
    'headers': {
        'test': 'abcd'
    },
    'maxRedirects': 20
};

var req = http.request(options, function (res) {
    var chunks = [];

    res.on("data", function (chunk) {
        chunks.push(chunk);
    });

    res.on("end", function (chunk) {
        var body = Buffer.concat(chunks);
        // console.log(body.toString());
    });

    res.on("error", function (error) {
        console.error(error);
    });
});

console.log(req.getHeaders());

req.end();

If you use console.log(req.headers); then it would print undefined in the above example.
Tested on both Node 16.x.x and 17.x.x versions

[mcollina]

That's not an IncomingMessage, that's a ClientRequest that inherits from OutgoingMessage (

node/lib/_http_client.js

Lines 112 to 113 in 646e057

	function ClientRequest(input, options, cb) {
	FunctionPrototypeCall(OutgoingMessage, this);

). You can see getHeaders() clearly documented there.

There is one mistake in the doc now that I see it: it states that ClientRequest inherits from Stream and not OutgoingMessage (https://nodejs.org/api/http.html#class-httpclientrequest).

[kcak11]

ok, so in that case the current pull-request is good I assume. Can you please approve.

[mcollina]

As I said above, it's not correct.

[benjamingr]

Hey @kcak11 thank you for your contribution to Node.js. As Matteo explained this change (headers -> getHeaders) isn't correct (I know this whole ClientRequest/IncomingMessage stuff might be confusing in the docs).

There are many other places you can contribute in the docs though - lots of APIs/events would benefit from examples and more explanations about what they do.

[kcak11]

Hey @kcak11 thank you for your contribution to Node.js. As Matteo explained this change (headers -> getHeaders) isn't correct (I know this whole ClientRequest/IncomingMessage stuff might be confusing in the docs).

There are many other places you can contribute in the docs though - lots of APIs/events would benefit from examples and more explanations about what they do.

Ok, I just fixed the extends for http.ClientRequest to point to http.OutgoingMessage

If thats ok for this PR to get merged, then it is fine. Otherwise I can close this PR and come back later.

[kcak11]

As I said above, it's not correct.

Fixed the error: f636bf8

[mcollina]

change is good! Can you squash the commits and update the PR title? Thanks!

[kcak11]

change is good! Can you squash the commits and update the PR title? Thanks!

I think I don't have permission to squash the commits. While merging the PR I guess it gives the option to Squash. Right now the Merge option is disabled for me.

[benjamingr]

@kcak11 you can squash the commits by going to your local checkout and running git rebase -i origin/master which will open an interactive rebase prompt. You mark all the comments you want into fold into the others as "fixup" (f in the rebase text file).

[kcak11]

7cc9ddf

Squashed into 7cc9ddf

I am not very familiar with rebase and I have used Github Desktop client for squashing the commits.

The Github Web interface gives the "Squash and Merge" option while merging the pull-request, so I believe we can use that instead of doing it from my CLI.

[benjamingr]

You accidentally merged instead of rebasing that (there are now 8 commits instead of 1). To float your head on top of the squashed commit 7cc9ddf you can:

# in your branch, reset to master
$ git reset --hard origin/master
# take just the commit
$ git cheery-pick 7cc9ddf68e89dc3627d6e826de091dc76483c200 
# push, overriding the status but only if no one else did
# assumes your remote is called "mine" but change to whatever you're using :) 
$ git push --force-with-lease mine HEAD

[kcak11]

You accidentally merged instead of rebasing that (there are now 8 commits instead of 1). To float your head on top of the squashed commit 7cc9ddf you can:

# in your branch, reset to master
$ git reset --hard origin/master
# take just the commit
$ git cheery-pick 7cc9ddf68e89dc3627d6e826de091dc76483c200 
# push, overriding the status but only if no one else did
# assumes your remote is called "mine" but change to whatever you're using :) 
$ git push --force-with-lease mine HEAD

Ran the same commands and it resulted in 5dde7af

Now I see 9 commits in the PR

[benjamingr]

That implies that you either didn't git reset --hard origin/master as the first step (since it's on top of the others) or that somehow your origin/master is corrupted (that is: you pushed these changed on to your local master branch and set your origin remote to your fork).

This is further implied by your branch name (also master) :] You need to reset to Node's master branch and base your changes on top of that - this probably means the first step git reset --hard origin/master needs to reflect that origin isn't Node's tree but yours. So probably something like this instead:

$ git remote add node https://github.com/nodejs/node/
$ git fetch node master
$ git reset --hard node/master

[kcak11]

That implies that you either didn't git reset --hard origin/master as the first step (since it's on top of the others) or that somehow your origin/master is corrupted (that is: you pushed these changed on to your local master branch and set your origin remote to your fork).

This is further implied by your branch name (also master) :] You need to reset to Node's master branch and base your changes on top of that - this probably means the first step git reset --hard origin/master needs to reflect that origin isn't Node's tree but yours. So probably something like this instead:

git remote add node https://github.com/nodejs/node/
git fetch node master
git reset --hard node/master

Thanks, it worked this time. Now there is a single commit with all the changes.

[mcollina]

lgtm

[ShogunPanda]

LGTM!

[kcak11]

@mcollina / @benjamingr / @ShogunPanda / @VoltrexMaster can one of you please merge the PR as all the checks have passed.

Thanks in advance.

[VoltrexKeyva]

@mcollina / @benjamingr / @ShogunPanda / @VoltrexMaster can one of you please merge the PR as all the checks have passed.

Thanks in advance.

Pull requests must wait at least 2 days before landing as stated here unless fast-tracked using the fast-track PRs that do not need to wait for 48 hours to land. label.

Which this PR can fast-tracked, if agreed by other collaborators.

[github-actions]

Fast-track has been requested by @benjamingr. Please 👍 to approve.

[nodejs-github-bot]

Landed in 28d8614

[benjamingr]

Congrats on your first commit @kcak11 🎉

[kcak11]

Congrats on your first commit @kcak11 🎉

Thanks @benjamingr

[kcak11]

Since I am a new contributor to this repo, just curious to know if I need to do anything else for the document to get updated. Right now I don't see the changes in this PR being reflected in the doc although there was a 17.9.0 release today.

Does the doc publish & update take more time ?

[benjamingr]

@kcak11 can take time, I assume the change is not in that version yet (though you can check the tag yourself and see) - non urgent (read: non security) changes aren't treated urgently :)

If you'd like: there is a lot of improvement potential in the docs:

• A lot of APIs are missing examples and some with examples are missing examples for certain parameters
• A lot of APIs can be better explained or organized

I also think @Trott who has been doing a lot of good docs work may have better intuition than I do about it :)

[kcak11]

@kcak11 can take time, I assume the change is not in that version yet (though you can check the tag yourself and see) - non urgent (read: non security) changes aren't treated urgently :)

If you'd like: there is a lot of improvement potential in the docs:

• A lot of APIs are missing examples and some with examples are missing examples for certain parameters
• A lot of APIs can be better explained or organized

I also think @Trott who has been doing a lot of good docs work may have better intuition than I do about it :)

Thanks for the information @benjamingr