Skip to content

http2: simplify custom promisify - #21835

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:simplify-custom-promisify
Closed

http2: simplify custom promisify#21835
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:simplify-custom-promisify

Conversation

@BridgeAR

Copy link
Copy Markdown
Member

This is just a small refactoring.

Refs: #20830 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This is just a small refactoring.
Refs: nodejs#20830 (comment)
@BridgeAR
BridgeAR requested a review from TimothyGuJuly 16, 2018 15:45
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Jul 16, 2018
@BridgeARBridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Jul 16, 2018

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this changes semantics -- previously, it would resolve the Promise once the connection is established, now it does so immediately.
I think the former is the expected behaviour here.

@addaleaxaddaleax removed the fast-track PRs that do not need to wait for 48 hours to land. label Jul 16, 2018
@BridgeAR

Copy link
Copy Markdown
MemberAuthor

@addaleax true. Even though there is another way of doing it that would still be more straight forward, I do not really think that it has to be changed. I just tried to follow up on the request. Closing therefore.

@BridgeAR
BridgeAR deleted the simplify-custom-promisify branch January 20, 2020 11:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@BridgeAR@nodejs-github-bot@addaleax