Skip to content

stream: writableCorked#29012

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.

Sign up for GitHub

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

Jump to bottom
Closed
ronag wants to merge 2 commits into from
Closed

stream: writableCorked #29012

ronag wants to merge 2 commits into from

Conversation

@ronag
Copy link
Member

@ronagronag commented Aug 6, 2019•
edited
Loading

Expose _writableState.corked.

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-botnodejs-github-bot added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. labels Aug 6, 2019
@ronag
Copy link
MemberAuthor

ronag commented Aug 6, 2019

Does this need a test?

@ronagronag mentioned this pull request Aug 6, 2019
Closed
8 tasks
@ronagronagforce-pushed the stream-writablecorked branch from 9d143cc to 268c091CompareAugust 6, 2019 11:44
@addaleax
Copy link
Member

addaleax commented Aug 6, 2019

Does this need a test?

Yes, almost all PRs that touch lib/ or src/ should have tests, if possible. And in particular, new APIs should always come with tests.

@addaleaxaddaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 6, 2019
@ronagronagforce-pushed the stream-writablecorked branch from 268c091 to e49d6a4CompareAugust 6, 2019 11:50
@ronag
Copy link
MemberAuthor

ronag commented Aug 6, 2019

Test added

@ronagronagforce-pushed the stream-writablecorked branch 2 times, most recently from 631ebb7 to 8358cb8CompareAugust 6, 2019 11:52
vsemozhetbyt
vsemozhetbyt reviewed Aug 6, 2019
View reviewed changes
@ronagronagforce-pushed the stream-writablecorked branch from 8358cb8 to f7fe3cfCompareAugust 6, 2019 13:03
@ronagronag mentioned this pull request Aug 6, 2019
Closed
9 tasks
@ronagronagforce-pushed the stream-writablecorked branch 2 times, most recently from e182b30 to e37e92eCompareAugust 6, 2019 20:32
jasnell
jasnell requested changes Aug 7, 2019
View reviewed changes
@ronagronagforce-pushed the stream-writablecorked branch 2 times, most recently from 41713d2 to 80cec35CompareAugust 9, 2019 16:03
mcollina
mcollina approved these changes Aug 28, 2019
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell is this ok now?

@mcollina
Copy link
Member

mcollina commented Aug 28, 2019

This needs a rebase.

@ronagronagforce-pushed the stream-writablecorked branch from 80cec35 to dca2160CompareAugust 28, 2019 07:51
@ronag
Copy link
MemberAuthor

ronag commented Aug 28, 2019

rebased

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 28, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/25283/

@mcollina
Copy link
Member

mcollina commented Sep 2, 2019

There were quite a few CI failures, would you mind taking a look?

@ronagronagforce-pushed the stream-writablecorked branch from dca2160 to 67dae03CompareSeptember 2, 2019 17:02
@ronag
Copy link
MemberAuthor

ronag commented Sep 2, 2019

another rebase

@ronag
Copy link
MemberAuthor

ronag commented Sep 2, 2019

@Trott: Travis problems?

22760.2 Compile V8 (py3) | errored -- | --

and then everything else is cancelled...

@ronagronag mentioned this pull request Nov 4, 2019
Closed
4 tasks
addaleax
addaleax approved these changes Nov 5, 2019
View reviewed changes
Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 5, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 5, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/26350/

@addaleax
Copy link
Member

addaleax commented Nov 5, 2019

Landed in de11913

@addaleaxaddaleax closed this Nov 5, 2019
addaleax pushed a commit that referenced this pull request Nov 5, 2019
@ronag@addaleax
stream: add writableCorked property
de11913
PR-URL: #29012 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
@ronag@MylesBorins
stream: add writableCorked property
c254d74
PR-URL: #29012 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Nov 19, 2019

@mcollina@jasnell@addaleax@ronag before we publish this in the next release: is the name what we want to stick with? .cork() and .uncork() are only available for writable streams, so maybe .corked as property name would have been sufficient? Just asking as I noticed the redundancy while putting together the changelog for v13.2.0.

@mcollina
Copy link
Member

mcollina commented Nov 19, 2019

@BridgeAR I think we should stick with the prefix for consistency. I'm also fine in dropping it.

@BridgeARBridgeAR mentioned this pull request Nov 19, 2019
Merged
@addaleax
Copy link
Member

addaleax commented Nov 19, 2019

I also don’t have a strong opinion on the name.

@ronag
Copy link
MemberAuthor

ronag commented Nov 19, 2019•
edited
Loading

I also think we should stick with consistency. We've already postfixed all the other writable specific properties. Changing the convention now feels unnecessary and unintuitive. I feel rather strongly about this.

MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
@MylesBorins
2019-11-21, Version 13.2.0 (Current)
8eda60c
Notable changes: * addons: * Deprecate one- and two-argument `AtExit()`. Use the three-argument variant of `AtExit()` or `AddEnvironmentCleanupHook()` instead (Anna Henningsen) nodejs#30227 * child_process,cluster: * The `serialization` option is added that allows child process IPC to use the V8 serialization API (to e.g., pass through data types like sets or maps) (Anna Henningsen) nodejs#30162 * deps: * Update V8 to 7.9 * Update `npm` to 6.13.0 (Ruy Adorno) nodejs#30271 * embedder: * Exposes the ability to pass cli flags / options through an API as embedder (Shelley Vohr) nodejs#30466 * Allow adding linked bindings to Environment (Anna Henningsen) nodejs#30274 * esm: * Unflag --experimental-modules (Guy Bedford) nodejs#29866 * stream: * Add `writable.writableCorked` property (Robert Nagy) nodejs#29012 * worker: * Allow specifying resource limits (Anna Henningsen) nodejs#26628 * v8: * The Serialization API is now stable (Anna Henningsen) nodejs#30234 PR-URL: nodejs#30547
MylesBorins added a commit that referenced this pull request Nov 21, 2019
@MylesBorins
2019-11-21, Version 13.2.0 (Current)
b8f8f05
Notable changes: * addons: * Deprecate one- and two-argument `AtExit()`. Use the three-argument variant of `AtExit()` or `AddEnvironmentCleanupHook()` instead (Anna Henningsen) #30227 * child_process,cluster: * The `serialization` option is added that allows child process IPC to use the V8 serialization API (to e.g., pass through data types like sets or maps) (Anna Henningsen) #30162 * deps: * Update V8 to 7.9 * Update `npm` to 6.13.0 (Ruy Adorno) #30271 * embedder: * Exposes the ability to pass cli flags / options through an API as embedder (Shelley Vohr) #30466 * Allow adding linked bindings to Environment (Anna Henningsen) #30274 * esm: * Unflag --experimental-modules (Guy Bedford) #29866 * stream: * Add `writable.writableCorked` property (Robert Nagy) #29012 * worker: * Allow specifying resource limits (Anna Henningsen) #26628 * v8: * The Serialization API is now stable (Anna Henningsen) #30234 PR-URL: #30547
targos pushed a commit that referenced this pull request Jan 13, 2020
@ronag@targos
stream: add writableCorked property
6a8a33c
PR-URL: #29012 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@targostargos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@ronag@BethGriggs
stream: add writableCorked property
af960d7
PR-URL: #29012 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott left review comments

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@addaleaxaddaleaxaddaleax approved these changes

+1 more reviewer

@vsemozhetbytvsemozhetbytvsemozhetbyt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.httpIssues or PRs related to the http subsystem.semver-minorPRs that contain new features and should be released in the next minor version.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ronag@addaleax@mcollina@nodejs-github-bot@BridgeAR@jasnell@Trott@vsemozhetbyt