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

stream: writableCorked #29012

Closed
wants to merge 2 commits into from
Closed

Conversation

@ronag
Copy link
Contributor

ronag commented Aug 6, 2019

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
@ronag

This comment has been minimized.

Copy link
Contributor Author

ronag commented Aug 6, 2019

Does this need a test?

@ronag ronag mentioned this pull request Aug 6, 2019
6 of 8 tasks complete
@ronag ronag force-pushed the nxtedition:stream-writablecorked branch from 9d143cc to 268c091 Aug 6, 2019
@addaleax

This comment has been minimized.

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.

@ronag ronag force-pushed the nxtedition:stream-writablecorked branch from 268c091 to e49d6a4 Aug 6, 2019
@ronag

This comment has been minimized.

Copy link
Contributor Author

ronag commented Aug 6, 2019

Test added

@ronag ronag force-pushed the nxtedition:stream-writablecorked branch 2 times, most recently from 631ebb7 to 8358cb8 Aug 6, 2019
@ronag ronag force-pushed the nxtedition:stream-writablecorked branch from 8358cb8 to f7fe3cf Aug 6, 2019
@ronag ronag mentioned this pull request Aug 6, 2019
2 of 9 tasks complete
@ronag ronag force-pushed the nxtedition:stream-writablecorked branch 2 times, most recently from e182b30 to e37e92e Aug 6, 2019
@ronag ronag force-pushed the nxtedition:stream-writablecorked branch 2 times, most recently from 41713d2 to 80cec35 Aug 7, 2019
Copy link
Member

mcollina left a comment

LGTM

@jasnell is this ok now?

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Aug 28, 2019

This needs a rebase.

@ronag ronag force-pushed the nxtedition:stream-writablecorked branch from 80cec35 to dca2160 Aug 28, 2019
@ronag

This comment has been minimized.

Copy link
Contributor Author

ronag commented Aug 28, 2019

rebased

@nodejs-github-bot

This comment has been minimized.

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Sep 2, 2019

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

@ronag ronag force-pushed the nxtedition:stream-writablecorked branch from dca2160 to 67dae03 Sep 2, 2019
@ronag

This comment has been minimized.

Copy link
Contributor Author

ronag commented Sep 2, 2019

another rebase

@ronag

This comment has been minimized.

Copy link
Contributor Author

ronag commented Sep 2, 2019

@Trott: Travis problems?

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

and then everything else is cancelled...

Co-Authored-By: Rich Trott <rtrott@gmail.com>
@Trott Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019
@ronag

This comment has been minimized.

Copy link
Contributor Author

ronag commented Sep 18, 2019

@jasnell: Do you want any further changes?

* {integer}

Number of times [`writable.uncork()`][stream-uncork] needs to be
called in order to fully uncork the stream.

This comment has been minimized.

Copy link
@jasnell

jasnell Sep 24, 2019

Member

minor nit: an example here would be nice

@ronag ronag mentioned this pull request Nov 4, 2019
4 of 4 tasks complete
Copy link
Member

addaleax left a comment

Still LGTM

@nodejs-github-bot

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 5, 2019

Landed in de11913

@addaleax addaleax closed this Nov 5, 2019
addaleax added a commit that referenced this pull request Nov 5, 2019
PR-URL: #29012
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 17, 2019
PR-URL: #29012
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeAR

This comment has been minimized.

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

This comment has been minimized.

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.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Nov 19, 2019
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
* 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
* stream:
  * Add `writable.writableCorked` property (Robert Nagy)
    nodejs#29012
* worker:
  * Allow specifying resource limits (Anna Henningsen)
    nodejs#26628
BridgeAR added a commit to BridgeAR/node that referenced this pull request Nov 19, 2019
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
* 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
BridgeAR added a commit to BridgeAR/node that referenced this pull request Nov 19, 2019
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
* 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
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Nov 19, 2019
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 `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
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 19, 2019

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

@ronag

This comment has been minimized.

Copy link
Contributor Author

ronag commented Nov 19, 2019

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
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 `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
MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
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 `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
MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
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
MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
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
MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
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
MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
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
MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.