GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: use a default callback for fs.close() by jasnell · Pull Request #37174 · nodejs/node

Conversation

@jasnell

@jasnell 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 [email protected]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@jasnell
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 <[email protected]>
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to file-system APIs and the fs module. label Feb 1, 2021
@jasnell 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

nodejs-github-bot commented Feb 1, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

juanarbol
juanarbol approved these changes Feb 1, 2021
View reviewed changes

@juanarbol juanarbol left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I like this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jasnell jasnell added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 1, 2021
@aduh95

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

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 and benjamingr reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina approved these changes Feb 2, 2021
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
RaisinTen
RaisinTen approved these changes Feb 2, 2021
View reviewed changes
benjamingr
benjamingr approved these changes Feb 2, 2021
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Lxxyx
Lxxyx approved these changes Feb 3, 2021
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Feb 3, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Feb 3, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca
lpinca approved these changes Feb 3, 2021
View reviewed changes
Comment thread doc/api/fs.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread doc/api/fs.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

jasnell and others added 2 commits February 4, 2021 10:40
@jasnell @lpinca
[Squash] nits
602d8ba
Co-authored-by: Luigi Pinca <[email protected]>
@jasnell @aduh95
[Squash] nit
e696b41
Co-authored-by: Antoine du Hamel <[email protected]>
@nodejs-github-bot

nodejs-github-bot commented Feb 4, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Feb 5, 2021

Copy link
Copy Markdown
Member Author

Landed in 7d0f680

benjamingr reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell jasnell closed this Feb 5, 2021
jasnell added a commit that referenced this pull request Feb 5, 2021
@jasnell
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 <[email protected]>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@jasnell @danielleadams
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 <[email protected]>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams added a commit that referenced this pull request Feb 16, 2021
@danielleadams
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
v15.9.0 proposal #37405
Closed
v15.9.0 proposal #37406
Merged
danielleadams added a commit that referenced this pull request Feb 17, 2021
@danielleadams
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
@danielleadams
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
@danielleadams
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 aduh95 mentioned this pull request Feb 20, 2021
lib: add chainErrors API to avoid error swallowing #37460
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@jasnell @targos
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 <[email protected]>

PR-URL: #37174
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
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
@benjamingr benjamingr benjamingr approved these changes
@lpinca lpinca lpinca approved these changes
@Lxxyx Lxxyx Lxxyx approved these changes
@aduh95 aduh95 aduh95 approved these changes
@juanarbol juanarbol juanarbol approved these changes
@RaisinTen RaisinTen RaisinTen approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. fs Issues and PRs related to file-system APIs and the fs module. 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.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@jasnell @nodejs-github-bot @aduh95 @mcollina @benjamingr @lpinca @Lxxyx @juanarbol @RaisinTen
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.