http: add http.ClientRequest.getRawHeaderNames()

[simov]

Fixes: #37641

As discussed here #37641 (comment) I'm submitting a patch to expose the raw header names being sent with the request.

I didn't updated the documentation because I'm not sure about the process for deprecating things since DEP0066 was introduced in v12 and this is v15.

[aduh95]

Could you add documentation please?

[benjamingr]

This looks like a good start - documentation and a test would be the next steps :)

[simov]

@aduh95, @benjamingr, I updated the deprecation message, and added docs for the getRawHeaderNames in ClientRequest. However, I'm noticing that documentation for the rest of the methods there is missing. It should be identical to the http.ServerResponse for all headers related methods.

@benjamingr there is a test in test-http-mutable-headers.js, that's the only place I've found that have tests related to those methods.

[benjamingr]

that's the only place I've found that have tests related to those methods.

That sounds reasonable - if you'd like I think there is value in adding a few more cases there - otherwise the test you added is probably fine for now and we can add more tests in a future PR.

[benjamingr]

Generally LGTM, ping @nodejs/http

[simov]

@benjamingr I added one additional assertion for testing the new method on the response object.

@aduh95, @mcollina all comments were addressed.

[aduh95]

Can we avoid calling %Object.keys% here? We can get the same info from %Object.values%, right? Also, if headersMap is null, wouldn't %Object.keys% throw?

Suggested change

	const headers = Array(ObjectKeys(headersMap).length);
	if (headersMap !== null) {
	const values = ObjectValues(headersMap);
	if (headerMap == null) return [];
	const values = ObjectValues(headersMap);
	const headers = Array(values.length);
	{

[aduh95]

We can also save the length to a variable to reuse it in the for loop.

[simov]

Good call, pushed 👍

[simov]

I missed the length comment, but I can add that if needed?

[simov]

Btw values.length is stored in l so that's only one assignment + accessing the property added.

[aduh95]

I think we can either:

1. store the value in a variable and read the variable to avoid looking up the property value several time.
2. access the property each time and rely on the compiler optimisations.

The current implementation access the property twice before putting its value in a variable, that seems unnecessary.

[simov]

The current implementation access the property twice before putting its value in a variable, that seems unnecessary.

Exactly. Not sure if one additional property access call on top changes anything, but I can update the code if you want. Your call.

[aduh95]

Yes, I'd prefer to see either of the approach described in my comment above.

[mcollina]

lgtm

[nodejs-github-bot]

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

[Trott]

Landed in 4686cb5

Thanks for reporting the issue and doing the work to fix it! 🎉