writeable stream: group all properties by antsmartian · Pull Request #31187 · nodejs/node

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

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

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 <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

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 <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

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 <luigipinca@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 <luigipinca@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Backport-PR-URL: nodejs#32164

Backport-PR-URL: #32164 PR-URL: #31187 Refs: #31144 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: #31236 Refs: #31187 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Backport-PR-URL: #32164 PR-URL: #31187 Refs: #31144 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

lib/_stream_writable.js Outdated Show resolved Hide resolved

lpinca reviewed Jan 5, 2020

View reviewed changes

ronag reviewed Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag reviewed Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag reviewed Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

antsmartian force-pushed the writable_refactor branch from 88f20ec to ad4b942 Compare January 5, 2020 13:03

lpinca approved these changes Jan 5, 2020

View reviewed changes

ronag reviewed Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag reviewed Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag reviewed Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

ronag reviewed Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated

Copy link

Member

ronag Jan 5, 2020

Choose a reason for hiding this comment

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

Accidental change?

lundibundi approved these changes Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

lib/_stream_writable.js Outdated Show resolved Hide resolved

lpinca approved these changes Jan 5, 2020

View reviewed changes

ronag reviewed Jan 5, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

BridgeAR approved these changes Jan 6, 2020

View reviewed changes

Trott approved these changes Jan 6, 2020

View reviewed changes

antsmartian mentioned this pull request Jan 7, 2020

stream: clean up definition using defineProperties #31236

Closed

4 tasks

lundibundi reviewed Jan 20, 2020

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

lundibundi removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 9, 2020

antsmartian force-pushed the writable_refactor branch from 2f226c8 to 5494213 Compare February 10, 2020 05:13

stream: combine properties using defineProperties

36c6219

antsmartian force-pushed the writable_refactor branch from 5494213 to 36c6219 Compare February 10, 2020 06:14

ronag approved these changes Feb 10, 2020

View reviewed changes

antsmartian added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2020

addaleax approved these changes Feb 10, 2020

View reviewed changes

addaleax closed this Feb 10, 2020

codebytere added the backport-requested-v13.x label Feb 29, 2020

MylesBorins mentioned this pull request Mar 9, 2020

Multiple backports for Streams needed on v13.x #32159

Closed

7 tasks

ronag mentioned this pull request Mar 9, 2020

[v13.x backport] writeable stream: group all properties #32164

Closed

4 tasks

MylesBorins added backported-to-v13.x and removed backport-requested-v13.x labels Mar 10, 2020

MylesBorins mentioned this pull request Mar 10, 2020

v13.11.0 proposal #32185

Merged

targos mentioned this pull request Apr 22, 2020

v12.16.3 release proposal #33009

Merged

Uh oh!

Conversation

antsmartian commented Jan 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag Jan 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antsmartian Jan 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

antsmartian commented Jan 5, 2020

Uh oh!

Uh oh!

Uh oh!

ronag Jan 5, 2020

Choose a reason for hiding this comment

Uh oh!

lundibundi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 11, 2020

Uh oh!

antsmartian commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 13, 2020

Uh oh!

Uh oh!

addaleax commented Feb 7, 2020

Uh oh!

ronag commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 10, 2020

Uh oh!

antsmartian commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Feb 10, 2020

Uh oh!

MylesBorins commented Mar 9, 2020

Uh oh!

mcollina commented Mar 9, 2020

Uh oh!

Reviewers

Assignees

antsmartian commented Jan 5, 2020 •

edited

Loading

ronag Jan 5, 2020 •

edited

Loading

antsmartian Jan 5, 2020 •

edited

Loading

antsmartian commented Jan 13, 2020 •

edited

Loading

ronag commented Feb 9, 2020 •

edited

Loading

antsmartian commented Feb 10, 2020 •

edited

Loading