Skip to content

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 11, 2019

Simplify and slightly optimize draining outgoing http streams.

Avoid extra event listener and inline with rest of the drain logic.

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

NOTE TO SELF: After this is merged add destroyed and ensure 'drain' is not emitted after destroy(). Also look into removing unnecessary flush class in write.

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 11, 2019
@mscdex
Copy link
Contributor

mscdex commented Aug 11, 2019

I think we should be using a symbol instead of an underscore-prefixed property.

@ronag ronag force-pushed the http-simplify-drain branch from 078268c to 054d050 Compare August 11, 2019 15:11
@ronag
Copy link
Member Author

ronag commented Aug 11, 2019

@mscdex: fixed

@ronag ronag force-pushed the http-simplify-drain branch 7 times, most recently from e82d80e to a6486de Compare August 11, 2019 15:57
@ronag ronag force-pushed the http-simplify-drain branch 4 times, most recently from e9184d5 to 35ba9c3 Compare August 11, 2019 19:35
@ronag ronag force-pushed the http-simplify-drain branch 3 times, most recently from 2a6fb63 to 41a0458 Compare August 13, 2019 13:47
@Trott
Copy link
Member

Trott commented Aug 13, 2019

@nodejs/http

@Trott
Copy link
Member

Trott commented Aug 16, 2019

@ronag Can you give this a rebase to get rid of the conflicts?

@nodejs/collaborators @nodejs/streams This could use some reviews.

@ronag ronag force-pushed the http-simplify-drain branch from 41a0458 to dcca00f Compare August 17, 2019 04:44
@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@Trott fixed

@ronag ronag force-pushed the http-simplify-drain branch from dcca00f to 73e760f Compare August 17, 2019 05:11
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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Aug 19, 2019

Landed in bdf07f4

@Trott Trott closed this Aug 19, 2019
Trott pushed a commit that referenced this pull request Aug 19, 2019
Simplify and slightly optimize draining outgoing http streams. Avoid
extra event listener and inline with rest of the drain logic.

PR-URL: #29081
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Aug 20, 2019
Simplify and slightly optimize draining outgoing http streams. Avoid
extra event listener and inline with rest of the drain logic.

PR-URL: #29081
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants