v19.1.0: breaking change, http.IncomingMessage headers setter no-op

[dnalborczyk]

Version

19.1.0

Platform

Darwin unifi 20.6.0 Darwin Kernel Version 20.6.0: Thu Sep 29 20:15:11 PDT 2022; root:xnu-7195.141.42~1/RELEASE_X86_64 x86_64

Subsystem

V8 or http

What steps will reproduce the bug?

an OSS project of mine does some integration testing with other libraries and tests involving 2 libraries (independently used) started failing when running with v19.1.0. after digging into the code, I came up with a reduced test case.

I haven't found time to bisect yet, I can't really tell what exactly causes the issue, although I'm suspecting V8 is the culprit. I'm also not sure what the intended behavior should be, e.g. assuming V8 is the cause of the issue, if it corrected some spec related behavior, or if it broke something.

const { IncomingMessage } = require("http")

class ServerlessRequest extends IncomingMessage {
  constructor() {
    super()

    Object.assign(this, {
      headers: {
        foo: "bar",
      },
    })
  }
}

const serverlessRequest = new ServerlessRequest()

console.log(Object.hasOwn(serverlessRequest, "headers"))
console.log("headers" in serverlessRequest)
console.log(serverlessRequest.headers)
console.log(serverlessRequest.headers?.foo)

node.js 19.0.1:
false
true
{ foo: 'bar' }
bar

node.js 19.1.0
false
true
{}
undefined

How often does it reproduce? Is there a required condition?

always

What is the expected behavior?

see above

What do you see instead?

see above

Additional information

I haven't found enough time yet to look for a smaller repro without involving IncomingMessage from the http module, with something like this mirroring the http implementation, but to no avail.

function IncomingMessage() {}

Object.defineProperty(IncomingMessage.prototype, "headers", {
  __proto__: null,

  get: function () {
    return {};
  },

  set: function (value) {},
});

[anonrig]

IncomingMessage headers setter became a no-op in the latest release. Referencing 4d723c7

I couldn't find a documentation change for this breaking change.

CC'ing reviewers for better understanding of this change: @mcollina @ShogunPanda @Trott

[dnalborczyk]

thank you @anonrig , good find. after following up on the PR discussion it appears to be a breaking change which would require a semver major and should likely be rolled back, although I agree with the change. that said, I think it would have been more consequent to remove the setter altogether.

[Trott]

@nodejs/http @sonimadhuri Any thoughts on the right thing to do here? I'm not sure which of these options (or maybe some other option) is the way to go:

1. Revert the change, reintroduce it as a breaking change, and get the word out to relevant maintainers before the next major release.
2. Revert the change, add minimal test coverage for the setter, document a legitimate use case or two in the docs.

[Trott]

Revert is in #45527.

I think the thing to do is re-apply this as a breaking change and put it in 20.x (and warn the nock maintainers and anyone else we think might be affected). But I'll likely defer to others on this.

[sonimadhuri]

@Trott I think all of us agree that the change is valid, so I think re-introducing this as semver major and updating documentation would be the right thing to do.