Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: make open and close stream override optional when unused #40013

Merged
merged 1 commit into from
Sep 16, 2021
Merged

fs: make open and close stream override optional when unused #40013

merged 1 commit into from
Sep 16, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 6, 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.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 6, 2021
@aduh95 aduh95 force-pushed the fs-streams-optional-open-close branch from c3a32ad to f1022f7 Compare September 6, 2021 09:39
@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 6, 2021
@aduh95 aduh95 force-pushed the fs-streams-optional-open-close branch 2 times, most recently from fbab685 to fe5f720 Compare September 6, 2021 10:54
mscdex
mscdex reviewed Sep 6, 2021
View reviewed changes
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Sep 6, 2021
View reviewed changes
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Sep 6, 2021
View reviewed changes
lib/internal/fs/streams.js Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Sep 6, 2021
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Sep 6, 2021
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Sep 6, 2021
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Sep 6, 2021
View reviewed changes
lib/internal/fs/streams.js
// 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');
Copy link
Member

Choose a reason for hiding this comment

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

The error message here could be more descriptive (could just use the same message as the comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

mscdex
mscdex reviewed Sep 6, 2021
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 8, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Sep 14, 2021
Open
Open
Open
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 16, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 16, 2021
@github-actions
Copy link
Contributor

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/.ncu
https://github.com/nodejs/node/actions/runs/1243428716

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 16, 2021
@aduh95
fs: make open and close stream override optional when unused
8a92018 
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 aduh95 force-pushed the fs-streams-optional-open-close branch from 52effa5 to 8a92018 Compare September 16, 2021 22:45
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 16, 2021

Landed in 8a92018

@aduh95 aduh95 merged commit 8a92018 into nodejs:master Sep 16, 2021
@aduh95 aduh95 deleted the fs-streams-optional-open-close branch September 16, 2021 22:47
This was referenced Sep 17, 2021
Open
Open
Open
Open
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@aduh95 @BethGriggs
fs: make open and close stream override optional when unused
e86a813 
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
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
45a042b 
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
@aduh95 @BethGriggs
fs: make open and close stream override optional when unused
59fff92 
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
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
10b4fe1 
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 BethGriggs mentioned this pull request Sep 21, 2021
Merged
1 task
@github-actions github-actions bot mentioned this pull request Sep 22, 2021
Open
15 tasks
BethGriggs added a commit that referenced this pull request Sep 22, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
aff2a0a 
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
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
8d5675e 
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
@ylemkimon
doc: note readStream.path if fd is specified
0fbdf07 
nodejs#40013 changed the behavior of `readStream.path`, it'll be `undefined` if `fd` is specified.
@ylemkimon ylemkimon mentioned this pull request Sep 29, 2021
Closed
Ayase-252 pushed a commit that referenced this pull request Oct 7, 2021
@ylemkimon @Ayase-252
doc: note readStream.path if fd is specified
b77e36c 
it'll be `undefined` if `fd` is specified.

Refs: #40013

PR-URL: #40252
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Oct 8, 2021
@ylemkimon @danielleadams
doc: note readStream.path if fd is specified
cff0c12 
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
Reviewers

@mscdex mscdex mscdex left review comments

@jasnell jasnell jasnell 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. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. semver-minor PRs 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.
4 participants
@aduh95 @nodejs-github-bot @mscdex @jasnell