writeable stream: group all properties #31187

antsmartian · 2020-01-05T05:34:26Z

Refs: #31144 for writable streams.

cc @nodejs/streams

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

lib/_stream_writable.js

nodejs-github-bot · 2020-01-05T08:04:45Z

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

lib/_stream_writable.js

ronag · 2020-01-05T12:08:37Z

lib/_stream_writable.js

-    if (this._writableState === undefined) {
-      return false;
+  destroyed: {
+    enumerable: false,


I think ObjectDefineProperties defaults enumerable to false, so I don't think these are needed? I'm not sure I understand the above comment about making it explicit? @BridgeAR any idea who is the original author of this comment?

@ronag: That's a good point. I guess we no need to add these properties. For example, I tested the below code and by default the enumerable property is false:

const a = {} Object.defineProperties(a, { test: { get() { return 'test'} } }) a.test // test a.propertyIsEnumerable('test') // false

Edit: Guess you have wrongly pinged instead of me :)

Seems like that was Calvin. Some people prefer explicit descriptor properties. Probably because not everyone is handling descriptors frequently and knows that their defaults are all false. It should be fine either way.

As this also came up in #31287 (comment):

I think it’s better to be explicit here, regardless of what people may know about the default values; partly because it removes cognitive overhead of figuring out what the behaviour is, partly because making it explicit forces code authors to be deliberate about the values they choose.

lib/_stream_writable.js

antsmartian · 2020-01-05T14:17:01Z

@ronag Done. @lpinca: Removed enumerable definition as mentioned here: #31187 (comment)

Could you PTAL again?

lib/_stream_writable.js

ronag · 2020-01-05T14:23:50Z

lib/_stream_writable.js

@@ -198,7 +199,7 @@ ObjectDefineProperty(WritableState.prototype, 'buffer', {
  get: internalUtil.deprecate(function writableStateBufferGetter() {
    return this.getBuffer();
  }, '_writableState.buffer is deprecated. Use _writableState.getBuffer ' +
-     'instead.', 'DEP0003')
+    'instead.', 'DEP0003')


Accidental change?

lundibundi

LGTM

lib/_stream_writable.js

lpinca

Still LGTM with nits addressed.

lib/_stream_writable.js

BridgeAR

LGTM with the comments addressed.

Trott

Pile-on "LGTM with nits addressed"

PR-URL: #31236 Refs: #31187 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>

BridgeAR · 2020-01-11T23:42:21Z

Ping @antsmartian

antsmartian · 2020-01-13T11:23:15Z

Sorry for delay, taken care the comments.. cc @BridgeAR @nodejs/streams

nodejs-github-bot · 2020-01-13T13:42:31Z

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

PR-URL: #31236 Refs: #31187 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>

lib/_stream_writable.js

addaleax · 2020-02-07T12:30:41Z

@antsmartian Can you sqaush the commits here together? CI detects a merge conflict in one of the earlier ones that is then resolved later, but it still fails to rebase because of that. (Also happy to do that for you if you prefer.)

ronag · 2020-02-09T16:20:53Z

Another property Writable.writable was added in e559842 which should probably also be included.

nodejs-github-bot · 2020-02-10T06:47:18Z

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

antsmartian · 2020-02-10T08:23:30Z

@addaleax PTAL.. This should be fine now.

addaleax · 2020-02-10T15:58:22Z

Landed in 0ac04ec 🙂

PR-URL: #31187 Refs: #31144 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2020-03-09T15:55:33Z

Hey All,

this doesn't land cleanly on v13.x should there be a backport?

mcollina · 2020-03-09T15:56:47Z

A backport will be good to have to avoid future conflicts.

PR-URL: nodejs#31187 Refs: nodejs#31144 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Backport-PR-URL: nodejs#32164

Backport-PR-URL: #32164 PR-URL: #31187 Refs: #31144 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #31236 Refs: #31187 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Backport-PR-URL: #32164 PR-URL: #31187 Refs: #31144 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 5, 2020

antsmartian force-pushed the writable_refactor branch 3 times, most recently from 52819a8 to 88f20ec Compare January 5, 2020 06:10

antsmartian commented Jan 5, 2020

View reviewed changes