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

Add closed property to OutgoingMessage. #15526

Closed
wants to merge 1 commit into from
Closed

Add closed property to OutgoingMessage. #15526

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 21, 2017

Similar to HTTP2ServerResponse.closed. This is an alternative to 8589c70 which provides an API similar to h2 compat and won't break existing OSS projects.

Fixes #15523

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
Affected core subsystem(s)

Similar to `HTTP2ServerResponse.closed`. This is an alternative to 8589c70 which provides an API similar to h2 compat and won't break existing OSS projects.
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Sep 21, 2017
@jasnell
Copy link
Member

jasnell commented Sep 21, 2017

ping @nodejs/http @mcollina

@mcollina
Copy link
Member

I'm -1 on this one. I still hope we can get writable = false in HTTP1, without breaking the ecosystem. It's worth spending time doing that, we should not add new properties lightly.

@ronag
Copy link
Member Author

ronag commented Sep 21, 2017

@mcollina: In that case, what do you think about removing closed from h2 compat? It should be essentially the same as writable? #15527

@mcollina
Copy link
Member

I'm 👍 in removing closed from HTTP2 compat layer if it's not on on HTTP1

@apapirovski
Copy link
Member

apapirovski commented Sep 21, 2017

To be clear, we should still track closed internally but not expose it — correct? (It's useful for internal stuff.)

@mscdex
Copy link
Contributor

mscdex commented Sep 21, 2017

If it's only used internally then it shouldn't be exposed to the public, use a Symbol or something instead.

@ronag
Copy link
Member Author

ronag commented Sep 21, 2017

before removing closed... do we have any way for the user to track it, e.g. writable with correct behavior (unlike h1)? #15527

@mcollina
Copy link
Member

@ronag I don't know. At this point we aim for compatibility, so I guess we would have to reproduce the "flaws" as well.

@BridgeAR
Copy link
Member

I think it is clear that this is not the way we want to move on and instead should just remove closed from http2 compat. Is everyone OK with closing this therefore?

@apapirovski
Copy link
Member

I have a PR open for http2 that removes it so 👍 from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http - res.closed
7 participants