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

http2 - compat "socket" #15313

Closed
ronag opened this issue Sep 10, 2017 · 1 comment
Closed

http2 - compat "socket" #15313

ronag opened this issue Sep 10, 2017 · 1 comment
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@ronag
Copy link
Member

ronag commented Sep 10, 2017

The compat socket getter should probably have a bit more null checks in order to be compat with old code:

i.e.

  get socket() {
    return this.stream && this.stream.session && this.stream.session.socket || undefined;
  }

instead of

  get socket() {
    return this.stream.session.socket;
  }

@apapirovski

@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 10, 2017
@apapirovski
Copy link
Member

Thanks for looking into this! There is a PR (#15254) open that deals with it in a slightly different way since a lot of existing implementations also try to access a property of the socket and so returning undefined is not enough.

apapirovski added a commit to apapirovski/node that referenced this issue Sep 12, 2017
Remove unnecessary variable assignments, remove unreachable code
pathways, remove path getter and setter, and other very minor
cleanup. Only remove reference to stream on nextTick so that
users and libraries can access it in the finish event.

Fixes: nodejs#15313
Refs: expressjs/express#3390
addaleax pushed a commit to addaleax/node that referenced this issue Sep 13, 2017
Remove unnecessary variable assignments, remove unreachable code
pathways, remove path getter and setter, and other very minor
cleanup. Only remove reference to stream on nextTick so that
users and libraries can access it in the finish event.

Fixes: nodejs#15313
Refs: expressjs/express#3390
PR-URL: nodejs#15254
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue Sep 20, 2017
Remove unnecessary variable assignments, remove unreachable code
pathways, remove path getter and setter, and other very minor
cleanup. Only remove reference to stream on nextTick so that
users and libraries can access it in the finish event.

Fixes: #15313
Refs: expressjs/express#3390
PR-URL: #15254
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants