Skip to content

stream: avoid setting writable state#31805

Closed
ronag wants to merge 2 commits intonodejs:masterfrom
nxtedition:stream-writable-avoid-writable
Closed

stream: avoid setting writable state#31805
ronag wants to merge 2 commits intonodejs:masterfrom
nxtedition:stream-writable-avoid-writable

Conversation

@ronag
Copy link
Member

@ronag ronag commented Feb 15, 2020 •
edited
Loading

A remainder from a previous refactoring.

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-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Feb 15, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2020

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

@ronag ronag requested a review from mcollina February 15, 2020 08:29
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2020

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

@ronag
Copy link
Member Author

ronag commented Feb 15, 2020

@Trott: Problems getting CI to pass:

tap2junit: error: argument --input/-i: can't open 'test.tap': [Errno 2] No such file or directory: 'test.tap'

Any advice?

@ronag ronag force-pushed the stream-writable-avoid-writable branch from e6a54b6 to 7b70470 Compare February 15, 2020 12:14
@ronag
stream: avoid writing to writable
8dd4e55 
A remainder from a previous refactoring.

Refs: nodejs#31197
@ronag ronag force-pushed the stream-writable-avoid-writable branch from 7b70470 to 8dd4e55 Compare February 15, 2020 12:21
@ronag ronag requested a review from lpinca February 15, 2020 12:22
mcollina
mcollina requested changes Feb 15, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

This needs unit tests

@ronag ronag requested a review from mcollina February 16, 2020 09:08
@ronag ronag force-pushed the stream-writable-avoid-writable branch from e9a79e3 to a712f4e Compare February 16, 2020 09:09
@ronag ronag force-pushed the stream-writable-avoid-writable branch from a712f4e to db24c25 Compare February 16, 2020 09:09
@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 16, 2020

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

@mcollina
Copy link
Member

mcollina commented Feb 16, 2020

I don't understand the reasoning behind this change, or what effect it would have on our APIs. Can you please elaborate? Note that the above link is a 404

@ronag
Copy link
Member Author

ronag commented Feb 16, 2020 •
edited
Loading

I don't understand the reasoning behind this change, or what effect it would have on our APIs. Can you please elaborate? Note that the above link is a 404

It doesn't change the API at all. We did a refactoring in #31197 to make Writable.writable computed. But we left a legacy mode to still allow writing to the property.

This PR doesn't change any API behavior. However, it fixes an unnecessary write to writable which would unnecessarily add a property on the state object. The observable state should remain the same. This should have been removed in #31197 but I missed it.

@mcollina
Copy link
Member

mcollina commented Feb 16, 2020

it fixes an unnecessary write to writable which would unnecessarily add a property on the state object

can you add a test for that?

@ronag
Copy link
Member Author

ronag commented Feb 16, 2020

can you add a test for that?

db24c25

@mcollina
Copy link
Member

mcollina commented Feb 16, 2020

I’m lost. You said it would save a call to write() and I cannot see how.

ronag
ronag commented Feb 16, 2020
View reviewed changes
lib/_stream_writable.js
onFinished(stream, state, cb);
}
state.ended = true;
stream.writable = false;
Copy link
Member Author

@ronag ronag Feb 16, 2020

Choose a reason for hiding this comment

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

@mcollina: The point is to remove this. It’s unnecessary (it should already be false through the computed getter) and causes the ‘writable’ property to be unnecessarily added on the state object.

@ronag ronag changed the title stream: avoid writing to writable stream: avoid setting writable property Feb 16, 2020
@ronag ronag changed the title stream: avoid setting writable property stream: avoid setting writable state Feb 16, 2020
mcollina
mcollina approved these changes Feb 16, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 17, 2020

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

@ronag ronag mentioned this pull request Feb 17, 2020
Closed
4 tasks
ronag added a commit that referenced this pull request Feb 17, 2020
@ronag
stream: avoid writing to writable
85c6fcd 
A remainder from a previous refactoring.

Refs: #31197

PR-URL: #31805
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ronag
Copy link
Member Author

ronag commented Feb 17, 2020

Landed in 85c6fcd

@ronag ronag closed this Feb 17, 2020
@BethGriggs BethGriggs mentioned this pull request Apr 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ronag @nodejs-github-bot @mcollina @jasnell @lpinca