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.

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

fs: require callback in read #22146

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Aug 6, 2018

Currently the read operation did not require the callback and that
was an oversight when callbacks became mandatory in 10.0.0.

I am not sure about the semverness in this case.

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

Currently the read operation did not require the callback and that
was an oversight when callbacks became mandatory.
@BridgeAR BridgeAR requested a review from a team August 6, 2018 00:04
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 6, 2018
@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 6, 2018
@Trott
Copy link
Member

Trott commented Aug 6, 2018

Labeling semver-major on the "if there's any doubt, choose major" approach, but if there's strong feelings going the other way, feel free to remove it.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 6, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 6, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM as semver-major

@trivikr
Copy link
Member

trivikr commented Aug 8, 2018

@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 9, 2018

I can't get any green builds recently. Either arm or node workers fail on multiple of my PRs. I would like to land this nevertheless as it is not related to either of the PRs. Is that fine?
(I resumed the builds multiple times without posting on multiple PRs).

BridgeAR added a commit to BridgeAR/node that referenced this pull request Aug 9, 2018
Currently the read operation did not require the callback and that
was an oversight when callbacks became mandatory.

PR-URL: nodejs#22146
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 9, 2018

Landed in 8e1b6e7 🎉

@BridgeAR BridgeAR closed this Aug 9, 2018
jasnell added a commit that referenced this pull request Oct 2, 2018
Notable changes:

* Build
  * FreeBSD 10 is no longer supported. [#22617](#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649)
  * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442)
* `crypto`
  * PEM-level encryption is now supported. [#23151](#23151)
  * An API for key pair generation has been added. [#22660](#22660)
* Dependencies
  * V8 has been updated to 7.0. [#22754](#22754)
* `fs`
  * The `fs.read()` method now requires a callback. [#22146](#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270)
* `http2`
  * An event will be emitted when a `PING` frame is received. [#23009](#23009)
  * Support for the `ORIGIN` frame has been added. [#22956](#22956)
* General
  * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating.
  * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951)
* Internal
  * Windows performance-counter support has been removed. [#22485](#22485)
  * The `--expose-http2` command-line option has been removed. [#20887](#20887)
* Promises
  * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218)
* Timers
  * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281)
  * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)
@jasnell jasnell mentioned this pull request Oct 2, 2018
4 tasks
jasnell added a commit that referenced this pull request Oct 17, 2018
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](#21914)
jasnell added a commit that referenced this pull request Oct 17, 2018
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](#21914)
jasnell added a commit that referenced this pull request Oct 21, 2018
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](#21914)
jasnell added a commit that referenced this pull request Oct 22, 2018
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](#21914)
devsnek pushed a commit to devsnek/node that referenced this pull request Oct 23, 2018
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [nodejs#21316](nodejs#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [nodejs#21649](nodejs#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [nodejs#20442](nodejs#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [nodejs#22754](nodejs#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [nodejs#22146](nodejs#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[nodejs#20735](nodejs#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [nodejs#20270](nodejs#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [nodejs#22951](nodejs#22951)
* Internal
  * Windows performance-counter support has been removed.
    [nodejs#22485](nodejs#22485)
  * The `--expose-http2` command-line option has been removed.
    [nodejs#20887](nodejs#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [nodejs#20002](nodejs#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [nodejs#22281](nodejs#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [nodejs#22756](nodejs#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [nodejs#21914](nodejs#21914)
deepak1556 pushed a commit to electron/node that referenced this pull request Dec 10, 2018
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](nodejs/node#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](nodejs/node#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](nodejs/node#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](nodejs/node#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](nodejs/node#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](nodejs/node#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](nodejs/node#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](nodejs/node#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](nodejs/node#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](nodejs/node#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](nodejs/node#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](nodejs/node#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](nodejs/node#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](nodejs/node#21914)
deepak1556 pushed a commit to electron/node that referenced this pull request Dec 19, 2018
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](nodejs/node#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](nodejs/node#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](nodejs/node#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](nodejs/node#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](nodejs/node#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](nodejs/node#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](nodejs/node#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](nodejs/node#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](nodejs/node#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](nodejs/node#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](nodejs/node#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](nodejs/node#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](nodejs/node#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](nodejs/node#21914)
@BridgeAR BridgeAR deleted the always-require-callbacks branch January 20, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.