Skip to content

Conversation

@ronag
Copy link
Member

@ronag ronag commented Aug 5, 2019

This is just a suggestion. Would like to see if there is any traction behind it.

It is quite common (at least in http and middleware) to check whether any further useful work can be performed on a writable stream. This is a bit similar to onFinished.isFinished.

We also already have it here: https://github.com/nodejs/node/blob/master/lib/internal/http2/compat.js#L304

Replaces #28628

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Aug 5, 2019
@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

ping @mcollina thoughts?

@ronag ronag force-pushed the stream-writablecomplete branch from bd7ad6f to 3999edb Compare August 5, 2019 09:44
@ronag ronag mentioned this pull request Aug 5, 2019
4 tasks
@ronag ronag force-pushed the stream-writablecomplete branch from 3999edb to 54b2d81 Compare August 5, 2019 09:53
@mcollina
Copy link
Member

mcollina commented Aug 5, 2019

I'm not sure about this one. I think writableFinished and destroyed provides this same capability. Why would we need this?

@ronag
Copy link
Member Author

ronag commented Aug 5, 2019

@mcollina: Convenience. Also the check is a bit more complicated on the http side of things. Not sure if there is a good user space way to check it there.

@Trott
Copy link
Member

Trott commented Aug 8, 2019

@nodejs/streams

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.

I'm -1 of adding this, as that state data is already exposed in other accessors

@ronag
Copy link
Member Author

ronag commented Aug 24, 2019

There is something with http here that might need further thinking but I can't remember what it was. Will close for now.

@ronag ronag closed this Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants