Skip to content

fs,net: standardize pending stream property - #24067

Closed
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:pending-all-the-things
Closed

fs,net: standardize pending stream property#24067
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:pending-all-the-things

Conversation

@addaleax

@addaleaxaddaleax commented Nov 3, 2018

Copy link
Copy Markdown
Member

Use the same property name as http2 does to indicate that
the stream is in the state before the ready event is emitted.

(This is semver-minor. If people feel that we should absolutely not have this in our public API, I’m probably going to incorporate this into a later PR using a Symbol rather than an “official” property.)

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

@addaleax
fs,net: standardize pending stream property
b4b0efe
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.
@addaleaxaddaleax added fs Issues and PRs related to the fs subsystem / file system. net Issues and PRs related to the net subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 3, 2018
@nodejs-github-bot

nodejs-github-bot commented Nov 3, 2018

Copy link
Copy Markdown
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1476/pipeline

@nodejs-github-botnodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. labels Nov 3, 2018
mscdex
mscdex reviewed Nov 3, 2018
View reviewed changes
Comment threaddoc/api/fs.md Outdated
mscdex
mscdex reviewed Nov 3, 2018
View reviewed changes
Comment threaddoc/api/net.md Outdated
mscdex
mscdex reviewed Nov 3, 2018
View reviewed changes
Comment threadlib/net.js
@addaleax

addaleax commented Nov 3, 2018

Copy link
Copy Markdown
MemberAuthor

@mscdex Done, addressed all 3 comments.

lpinca
lpinca reviewed Nov 3, 2018
View reviewed changes
Comment threadlib/internal/fs/streams.js Outdated
Comment threadtest/parallel/test-fs-ready-event-stream.js Outdated
Comment threadtest/parallel/test-fs-ready-event-stream.js Outdated
Comment threadtest/parallel/test-net-connect-buffer.js
@addaleax

addaleax commented Nov 3, 2018

Copy link
Copy Markdown
MemberAuthor

@lpinca Thanks for catching those, done!

sindresorhus
sindresorhus reviewed Nov 5, 2018
View reviewed changes
Comment threaddoc/api/fs.md
`readStream.path` will be a string. If `path` is passed as a `Buffer`, then
`readStream.path` will be a `Buffer`.

### readStream.pending

@sindresorhussindresorhusNov 5, 2018•
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would be better to name this .isPending. When you read .pending, it could either mean it contains a pending object or a boolean of whether it's pending. That's not clear from just reading the code.

@mscdexmscdexNov 5, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.pending I think fits the style of other pre-existing properties like .connecting

@addaleaxaddaleaxNov 5, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It’s also what HTTP/2 already uses for this purpose…

@addaleax

addaleax commented Nov 7, 2018

Copy link
Copy Markdown
MemberAuthor

Any other thoughts/reviewers?

@addaleax

addaleax commented Nov 10, 2018

Copy link
Copy Markdown
MemberAuthor

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

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 10, 2018
@addaleax

addaleax commented Nov 13, 2018

Copy link
Copy Markdown
MemberAuthor

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

@Trott

Trott commented Nov 14, 2018

Copy link
Copy Markdown
Member

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18605/

@Trott

Trott commented Nov 14, 2018

Copy link
Copy Markdown
Member

Landed in 0e06b35

@TrottTrott closed this Nov 14, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 14, 2018
@addaleax@Trott
fs,net: standardize pending stream property
0e06b35
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.
PR-URL: nodejs#24067
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
@addaleax@BridgeAR
fs,net: standardize pending stream property
a230139
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.
PR-URL: #24067
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
@addaleax@kiyomizumia
fs,net: standardize pending stream property
3cbc6cb
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.
PR-URL: nodejs#24067
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 15, 2018
@addaleax@BridgeAR
fs,net: standardize pending stream property
2aa23cd
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.
PR-URL: #24067
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeARBridgeAR mentioned this pull request Nov 15, 2018
Merged
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
@addaleax@BethGriggs
fs,net: standardize pending stream property
3b807e9
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.
PR-URL: #24067
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@addaleax@BethGriggs
fs,net: standardize pending stream property
9e99e3e
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.
PR-URL: #24067
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggsBethGriggs mentioned this pull request May 1, 2019
Merged
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@addaleax@MylesBorins
fs,net: standardize pending stream property
ae465f6
Use the same property name as http2 does to indicate that
the stream is in the state before the `ready` event is emitted.
PR-URL: #24067
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@JungMinuJungMinuJungMinu approved these changes
+2 more reviewers
@mscdexmscdexmscdex left review comments
@sindresorhussindresorhussindresorhus 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.fsIssues and PRs related to the fs subsystem / file system.http2Issues or PRs related to the http2 subsystem.netIssues and PRs related to the net subsystem.semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@addaleax@nodejs-github-bot@Trott@mscdex@sindresorhus@jasnell@lpinca@JungMinu