http: headers(Distinct), trailers(Distinct) setters to be no-op

[sonimadhuri]

No description provided.

[Trott]

For reviewers: I believe the intention of this test is to add coverage for line 145 at https://coverage.nodejs.org/coverage-fdadea8f6e42cea4/lib/_http_incoming.js.html#L145.

@nodejs/testing @nodejs/http

[nodejs-github-bot]

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

[ShogunPanda]

While the test LGTM, I now wonder if we want that property to be set by the user or just used by us internally.

[sonimadhuri]

agreed @ShogunPanda. Internally there wasn't any usage of the setter and I can/t think of a good reason why users might have to set it explicitly.

[Trott]

While the test LGTM, I now wonder if we want that property to be set by the user or just used by us internally.

Ideally, would an attempt to set it be a no-op? Throw an error? Be undefined behavior? Something else?

[Trott]

@sonimadhuri I think the thing to do for now might be to change the setter behavior to be a no-op and then update this test to check that the result doesn't change when something is assigned to it. Would you be willing to make that change?

If anyone wants to make the case that an error or warning should occur, that can always be added in later pull requests by anyone who wants to make it happen. (I like the idea of emitting a warning personally, but I'm not sure what others think. A no-op for now seems like the safest thing to do.)

Another option would be to land this as it is (because it tests existing behavior) and then open a pull request to change the behavior and the test. But I think changing the behavior now is probably appropriate.

[ShogunPanda]

@Trott I agree, let's change to no-op so that no-one will be affected.

Actually I would also go further: let's keep this behavior consistent between headers, headersDistinct, trailers and trailersDistinct.

[sonimadhuri]

@Trott I'll make this change and update the test. Should I make the changes for headers, trailers, trailersDistinct in the same PR then?

[Trott]

@Trott I'll make this change and update the test. Should I make the changes for headers, trailers, trailersDistinct in the same PR then?

Yes, I think doing that all at once makes sense. We may need to talk about the semver-ness of this change, but regardless, we should land that change on the main branch, so if you'd make those changes, that would be great!

[ShogunPanda]

LGTM!

[mcollina]

lgtm

[Trott]

@mcollina@ShogunPanda Do you think this is semver patch or semver major? And do you think this should be documented in doc/api/http.md somewhere?

[nodejs-github-bot]

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

[mcollina]

I think this is semver major.

[ShogunPanda]

I think is semver-minor instead.

The outfacing API hasn't changed and the writability of the fields was never documented.

If we speak about backward API compatibility, the change is compatible as what application get after parsing requests/response has not changed, if we exclude eventual fields set by the user (which was private/undocumented).

[mcollina]

+1 on a minor but let’s flag it as “baking for LTS” and see if it breaks somebody before hack porting?

[ShogunPanda]

I agree!

[nodejs-github-bot]

Landed in 4d723c7

[aduh95]

I've received a report of a failure seemingly related to this change (tus/tus-node-server#320 (comment)), marking as dont-land as a precaution.

[panva]

+1 on a minor but let’s flag it as “baking for LTS” and see if it breaks somebody before hack porting?

HTTP server mocking and expectations library for Node.js nock (npm) doesn't set response headers on 19.1.0 anymore.

[Trott]

Given the two comments here and the issue in #45510 (where I think further discussion should go), I think a revert is in order. But what to do after the revert...well, I left a comment in #45510. (UPDATE: I think we should revert and then re-land as a semver-major.)

[ShogunPanda]

I'm still don't really agree is a semver-major, as the API was undocumented and, IMHO, private and thus not subject to semver).
If possible, I'd love to have documentation updated instead.
Otherwise revert but make sure to include in 20.

[ljharb]

Anything reachable is public.

[ShogunPanda]

I strongly disagree on this, given the nature of JavaScript.
Unless something is clearly documented, I would assume that use an API is risky and therefore not necessarily subject to explicit support or semver coverage.

Now, with this I don't want to say that we can do whatever we want with node internals because people are relying on our actions, obviously. So we try to be as conservative as we possibly can.
But, in cases like this, I think the setters were leftovers that might lead to unexpected behaviors and, therefore, should be removed.

[ljharb]

The nature of JavaScript is that closures, and class private fields, are the only way to make anything "private". Everything else is public.

[Trott]

Given that there's consensus that it is acceptable to re-land this as a semver-major (even if there's disagreement on whether that's preferable), I would love to see us not use this PR to discuss what is and isn't public API. Or if that discussion must happen, maybe it can happen over in #45510 so that we don't splinter the conversation. Or even better, maybe the "what is/isn't public" question can be discussed in its own dedicated issue.