fs: make open and close stream override optional when unused
#40013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
fs
aduh95
force-pushed
the
fs-streams-optional-open-close
branch
from
c3a32ad
to
f1022f7
Compare
aduh95
added
the
semver-minor
aduh95
force-pushed
the
fs-streams-optional-open-close
branch
2 times, most recently
from
fbab685
to
fe5f720
Compare
mscdex
reviewed
Sep 6, 2021
mscdex
reviewed
Sep 6, 2021
mscdex
reviewed
Sep 6, 2021
mscdex
reviewed
Sep 6, 2021
mscdex
reviewed
Sep 6, 2021
mscdex
reviewed
Sep 6, 2021
jasnell
reviewed
Sep 6, 2021
| // When fd is a FileHandle we can listen for 'close' events | ||
| if (options.fs) { | ||
| // FileHandle is not supported with custom fs operations | ||
| throw new ERR_METHOD_NOT_IMPLEMENTED('FileHandle with fs'); |
There was a problem hiding this comment.
The error message here could be more descriptive (could just use the same message as the comment)
There was a problem hiding this comment.
The error message currently is The FileHandle with fs method is not implemented. ERR_METHOD_NOT_IMPLEMENTED may not be the fittest here, but I'm afraid changing it would be a breaking change, best left to another PR. Or do you have a suggestion that would work with ERR_METHOD_NOT_IMPLEMENTED?
jasnell
approved these changes
Sep 6, 2021
mscdex
reviewed
Sep 6, 2021
aduh95
added
author ready
github-actions
bot
removed
the
request-ci
This was referenced Sep 14, 2021
aduh95
added
the
commit-queue
github-actions
bot
removed
the
commit-queue
Commit Queue failed- Loading data for nodejs/node/pull/40013 ✔ Done loading data for nodejs/node/pull/40013 ----------------------------------- PR info ------------------------------------ Title fs: make `open` and `close` stream override optional when unused (#40013) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch aduh95:fs-streams-optional-open-close -> nodejs:master Labels fs, semver-minor, author ready, needs-ci Commits 7 - fs: make `open` and `close` stream override optional when unused - fixup! fs: make open and close stream override optional when unused - fixup! fs: make `open` and `close` stream override optional when unused - fixup! fs: make `open` and `close` stream override optional when unused - fixup! fs: make `open` and `close` stream override optional when unused - fixup! fs: make `open` and `close` stream override optional when unused - fixup! fs: make open and close stream override optional when unused Committers 2 - Antoine du Hamel - GitHub PR-URL: https://github.com/nodejs/node/pull/40013 Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/40013 Reviewed-By: James M Snell -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - fixup! fs: make open and close stream override optional when unused ℹ This PR was created on Mon, 06 Sep 2021 09:38:11 GMT ✔ Approvals: 1 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/40013#pullrequestreview-747279667 ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2021-09-12T22:47:33Z: https://ci.nodejs.org/job/node-test-pull-request/39931/ - Querying data for job/node-test-pull-request/39931/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/1243428716 |
github-actions
bot
added
the
commit-queue-failed
When using `createReadStream` or `createWriteStream` with a specific file descriptor or `FileHandle` instead of a path, `open` method is not used, there is no point in forcing users to provide it. When using `createReadStream` or `createWriteStream` with `autoClose` set to false, `close` method is not used, there is no point in forcing users to provide it. PR-URL: nodejs#40013 Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95
force-pushed
the
fs-streams-optional-open-close
branch
from
52effa5
to
8a92018
Compare
|
Landed in 8a92018 |
This was referenced Sep 17, 2021
BethGriggs
pushed a commit
that referenced
this pull request
Sep 21, 2021
When using `createReadStream` or `createWriteStream` with a specific file descriptor or `FileHandle` instead of a path, `open` method is not used, there is no point in forcing users to provide it. When using `createReadStream` or `createWriteStream` with `autoClose` set to false, `close` method is not used, there is no point in forcing users to provide it. PR-URL: #40013 Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs
added a commit
that referenced
this pull request
Sep 21, 2021
Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: TODO
BethGriggs
pushed a commit
that referenced
this pull request
Sep 21, 2021
When using `createReadStream` or `createWriteStream` with a specific file descriptor or `FileHandle` instead of a path, `open` method is not used, there is no point in forcing users to provide it. When using `createReadStream` or `createWriteStream` with `autoClose` set to false, `close` method is not used, there is no point in forcing users to provide it. PR-URL: #40013 Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs
added a commit
that referenced
this pull request
Sep 21, 2021
Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: TODO
15 tasks
BethGriggs
added a commit
that referenced
this pull request
Sep 22, 2021
Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: #40175
BethGriggs
added a commit
that referenced
this pull request
Sep 22, 2021
Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: #40175
ylemkimon
added a commit
to ylemkimon/node
that referenced
this pull request
Sep 29, 2021
nodejs#40013 changed the behavior of `readStream.path`, it'll be `undefined` if `fd` is specified.
danielleadams
pushed a commit
to danielleadams/node
that referenced
this pull request
Oct 8, 2021
it'll be `undefined` if `fd` is specified. Refs: nodejs#40013 PR-URL: nodejs#40252 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using
createReadStreamorcreateWriteStreamwith a specificfile descriptor or
FileHandleinstead of a path,openmethod is notused, there is no point in forcing users to provide it.
When using
createReadStreamorcreateWriteStreamwithautoCloseset to false,
closemethod is not used, there is no point in forcing usersto provide it.