FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: use a default callback for fs.close() by jasnell · Pull Request #37174 · nodejs/node · GitHub

/ node Public

fs: use a default callback for fs.close() - #37174

Closed
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:default-fs-close-callback
ConversationCommits (3)ChecksFiles changed
Closed

fs: use a default callback for fs.close()#37174
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:default-fs-close-callback

Conversation

jasnell commented Feb 1, 2021

Copy link
Copy Markdown
Member

The fs.close() function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

fs.close(fd);

// is equivalent to

fs.close(fd, (err) => {
  if (err != null) throw err;
});

/cc @mcollina

Signed-off-by: James M Snell jasnell@gmail.com

fs: use a default callback for fs.close()
61c243f 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>
nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 1, 2021
jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 1, 2021

nodejs-github-bot commented Feb 1, 2021

Copy link
Copy Markdown
Collaborator

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

juanarbol approved these changes Feb 1, 2021
View reviewed changes

juanarbol left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I like this

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 1, 2021

aduh95 commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

Note that this was the behaviour until Node.js v7.0.0 (deprecated in #7897 and removed in #12562).
Should we make the change for other functions as well? I think we can make the same assumption for fs.copyFile, fs.rename, fs.rm, fs.unlink, fs.writeFile, etc.

jasnell commented Feb 1, 2021

Copy link
Copy Markdown
Member Author

Should we make the change for other functions as well?

I wouldn't. fs.close() is a bit special since errors there are fairly rare. There are really good reasons why we'd want to require a callback be provided in those cases to ensure that things are handled appropriately. With errors on close, however, most often all you want to do is crash.

aduh95 approved these changes Feb 1, 2021
View reviewed changes
Comment thread lib/fs.js Outdated
mcollina approved these changes Feb 2, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

benjamingr approved these changes Feb 2, 2021
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is also good because it follows the same behaviour as fsPromises :]

nodejs-github-bot commented Feb 3, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Feb 3, 2021

Copy link
Copy Markdown
Collaborator

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

lpinca approved these changes Feb 3, 2021
View reviewed changes
Comment thread doc/api/fs.md Outdated
Comment thread doc/api/fs.md Outdated
jasnell and others added 2 commits February 4, 2021 10:40
[Squash] nits
602d8ba 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
[Squash] nit
e696b41 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot commented Feb 4, 2021

Copy link
Copy Markdown
Collaborator

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

jasnell commented Feb 5, 2021

Copy link
Copy Markdown
Member Author

Landed in 7d0f680

jasnell closed this Feb 5, 2021
jasnell added a commit that referenced this pull request Feb 5, 2021
fs: use a default callback for fs.close()
7d0f680 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
fs: use a default callback for fs.close()
655d196 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams added a commit that referenced this pull request Feb 16, 2021
2021-02-16, Version 15.9.0 (Current)
5133659 
Notable Changes:

* crypto:
  * add keyObject.export() 'jwk' format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
This was referenced Feb 16, 2021
Closed
Merged
danielleadams added a commit that referenced this pull request Feb 17, 2021
2021-02-17, Version 15.9.0 (Current)
889ffde 
Notable Changes:

* crypto:
  * add keyObject.export() 'jwk' format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 17, 2021
2021-02-17, Version 15.9.0 (Current)
877f66e 
PR-URL: #37406

Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 18, 2021
2021-02-17, Version 15.9.0 (Current)
3bf50a7 
PR-URL: #37406

Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
aduh95 mentioned this pull request Feb 20, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
fs: use a default callback for fs.close()
92348a9 
The `fs.close()` function requires a callback. Most often the only thing
that callback does is check and rethrow the error if one occurs. To
eliminate common boilerplate, make the callback optional with a default
that checks and rethrows the error as an uncaught exception.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams mentioned this pull request May 3, 2021
Merged
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
Lxxyx Lxxyx approved these changes
aduh95 aduh95 approved these changes
juanarbol juanarbol approved these changes
RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. fs Issues and PRs related to the fs subsystem / file system. 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.

9 participants

Back | FazBrowse Home | New Git URL