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

Accidental breaking change on v15.7.0 #37705

Closed
mmarchini opened this issue Mar 11, 2021 · 5 comments · Fixed by #37706
Closed

Accidental breaking change on v15.7.0 #37705

mmarchini opened this issue Mar 11, 2021 · 5 comments · Fixed by #37706

Comments

@mmarchini
Copy link
Contributor

mmarchini commented Mar 11, 2021

  • Version: v15.7.0
  • Platform: OS X (should be reproducible on any OS)
  • Subsystem: http2

What steps will reproduce the bug?

This issue was introduced on #36505

Try to assign something to req on an instance of Http2ServerResponse.
Example error: https://github.com/restify/node-restify/runs/2081960519?check_suite_focus=true#step:5:242

TypeError: Cannot set property req of [object Object] which has only a getter
    at Server._setupRequest (/home/runner/work/node-restify/node-restify/lib/server.js:1282:13)
    at Server._onRequest (/home/runner/work/node-restify/node-restify/lib/server.js:967:10)
    at Http2SecureServer.emit (node:events:378:20)
    at Http2SecureServer.EventEmitter.emit (node:domain:532:15)
    at Http2SecureServer.onServerStream (node:internal/http2/compat:870:10)
    at Http2SecureServer.emit (node:events:378:20)
    at Http2SecureServer.EventEmitter.emit (node:domain:532:15)
    at ServerHttp2Session.sessionOnStream (node:internal/http2/core:2930:19)
    at ServerHttp2Session.emit (node:events:378:20)
    at ServerHttp2Session.EventEmitter.emit (node:domain:532:15)

Example code: https://github.com/restify/node-restify/blob/master/lib/server.js#L1282

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

Every time

What is the expected behavior?

The code should not break on a semver-minor

What do you see instead?

An error is thrown

Additional information

This happens because getters are not assignable on strict mode. We can't revert the change because that would also be a breaking change, so we have two options:

  • Change it from a getter to a normal property, so that user code doesn't break. We can go back to a getter on v16
  • Leave as is, assuming that users shouldn't be assigning to the response object anyway and because v15 is not an LTS version, and then add a note to the changelogs that this was an accidental breaking change that won't be reverted

If we go with the latter we might want to discuss making all properties on req/res non-assignable (which IMO is not desirable).

cc @nodejs/tsc @nodejs/http2 @nodejs/releasers

@ghermeto
Copy link

cc/ @nodejs/web-server-frameworks

cjihrig added a commit to cjihrig/node that referenced this issue Mar 11, 2021
The change in nodejs#36505 broke
userland code that already wrote to res.req. This commit updates
the res.req property in the http2 compat layer to be a normal
property.

Fixes: nodejs#37705
@targos
Copy link
Member

targos commented Mar 12, 2021

@mmarchini do you think this is just an unfortunate breakage because due the popularity of theres.req = req pattern?
Should we start to treat every addition of a non-writable property to a public object as semver-major?

@mcollina
Copy link
Member

Should we start to treat every addition of a non-writable property to a public object as semver-major?

I think so, yes. It's very easy to support backward compatible behavior anyway.

@mmarchini
Copy link
Contributor Author

I agree with @mcollina, it's easy enough to ensure backward compatible behavior that we should avoid breaking it whenever possible.

@Ethan-Arrowood
Copy link
Contributor

I'm +1 for treating it as semver major since if someone is currently assigning to the property, making it readonly will break their code

danielleadams pushed a commit that referenced this issue Mar 16, 2021
The change in #36505 broke
userland code that already wrote to res.req. This commit updates
the res.req property in the http2 compat layer to be a normal
property.

PR-URL: #37706
Fixes: #37705
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Mary Marchini <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this issue Mar 16, 2021
The change in #36505 broke
userland code that already wrote to res.req. This commit updates
the res.req property in the http2 compat layer to be a normal
property.

PR-URL: #37706
Fixes: #37705
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Mary Marchini <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants