Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

sonimadhuri
Copy link
Contributor

No description provided.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Oct 25, 2022
@Trott
Copy link
Member

Trott commented Oct 26, 2022

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

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2022
@nodejs-github-bot
Copy link
Collaborator

@ShogunPanda
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

Trott commented Oct 26, 2022

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
Copy link
Member

Trott commented Oct 27, 2022

@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
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Member

Trott commented Oct 27, 2022

@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!

@sonimadhuri sonimadhuri force-pushed the tests/set_headers_distinct branch 2 times, most recently from 29db476 to 56fb2f4 Compare October 28, 2022 17:25
@Trott Trott changed the title test: add test case for setting headers distinct http: headers(Distinct), trailers(Distinct) setters to be no-op Oct 28, 2022
lib/_http_incoming.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Trott
Copy link
Member

Trott commented Oct 29, 2022

@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?

@Trott Trott added http Issues or PRs related to the http subsystem. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 29, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2022
@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

I think this is semver major.

@ShogunPanda
Copy link
Contributor

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).

@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 29, 2022
@mcollina
Copy link
Member

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

@Trott Trott added baking-for-lts PRs that need to wait before landing in a LTS release. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 30, 2022
@ShogunPanda
Copy link
Contributor

I agree!

@Trott Trott added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 31, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2022
@nodejs-github-bot nodejs-github-bot merged commit 4d723c7 into nodejs:main Oct 31, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 4d723c7

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
PR-URL: #45176
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 9, 2022
PR-URL: nodejs#45176
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
PR-URL: #45176
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 aduh95 added dont-land-on-v14.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Nov 17, 2022
@aduh95
Copy link
Contributor

aduh95 commented Nov 17, 2022

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
Copy link
Member

panva commented Nov 19, 2022

+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
Copy link
Member

Trott commented Nov 19, 2022

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
Copy link
Contributor

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
Copy link
Member

ljharb commented Nov 19, 2022

Anything reachable is public.

@ShogunPanda
Copy link
Contributor

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
Copy link
Member

ljharb commented Nov 20, 2022

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

@Trott
Copy link
Member

Trott commented Nov 20, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants