fs: remove maybeCallback function by thefourtheye · Pull Request #7168 · nodejs/node · GitHub

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

The "fs" module has two functions called maybeCallback and
makeCallback, as of now.

The maybeCallback creates a default function to report errors, if the
parameter passed is not a function object. Basically, if the callback
is omitted in some cases, this function is used to create a default
callback function.

The makeCallback, OTOH, creates a default function only if the
parameter passed is undefined, and if it is not a function object it
will throw an Error.

This patch removes the maybeCallback function and makes the callback
function argument mandatory for all the async versions.

cc @nodejs/collaborators

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

Generally I like this change but I'm not really sure it's worth it given the amount of breakage it might cause in the wild.

---I tried running a smoke test but the CI gets frozen "Loading" when I attempt it - but I definitely think it's worth running in this case---

Smoke test: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/294/

Code changes themselves LGTM.

I think the potential for ecosystem fallout is a bit too high to justify.

Note that makeCallback() and maybeCallback() treat the callback argument differently. The former calls rethrow() when it strict-equals undefined, the latter does so for any falsy value.

You could change makeCallback() to behave like maybeCallback() but that might introduce issues of its own.

If we're going to break stuff, I'd rather break it to throw immediately when no callback is passed. Instead of throwing sometime in the future when the stack has been lost.

@thealphanerd I looked at the smoke test output - it's green but looks like there are errors in the console output - can you PTAL?

some of our production code would break with this change...

get ready to cringe: that's because with a preflight stat verifying path or read/writability, it turns out fs read/writes done in the fire-and-forget fashion (never passing a callback) are not only fine but also reliable

@reqshark wait, what??

While not passing a callback might be equal to passing undefined, and most of the node API won't let me get away with this now, I liked when we had the option to ignore callbacks or their parameters.

Occasionally program errors should be ignored, and the ability to not have to write an error handler for an inconsequential routine will reduce lines of code... for example, writing a noop() might be overkill

The decision not to pass the callback should result in an immediate consequence, either throw immediately or its /dev/null. Passing a function, on the otherhand, indicates a clear statement about wanting to wait and handle that outcome.

If we are going to make a semver-major change here can we please simply throw immediately? Throwing in the future is one of the dumbest designs in node today.

Should it not throw with arguments.length being 1?

+1 to throwing immediately.

Thinking out loud: I do get the case where an explicit noop would be useful, however. I'm wondering if some form of sentinel value could be used to explicitly signal that the async method should simply not bother attempting to callback tho. A Symbol would work...

fs.writeFile('/tmp/foo', 'hello there', fs.NoCallback);

I dunno, just a thought.

@jasnell Seems like a better option for a userland module to do?

Okay, updated the PR to throw if the callback is not passed. PTAL.

Sweet. The code change LGTM. Looks like we're missing test coverage checking if all the altered functions throw. Mind making sure there's a single check for each method to make sure it throws if no callback is passed?

@trevnorris I included a test now to check if the functions fail when callback is not passed.

hmm... not thrilled about this approach. For instance, the following is technically not an incorrect way of calling the fs.access() function even if the extraneous arguments on the end aren't supported. This code, however, would fail:

fs.access('/some/path', () => {}, 'useless', 'arguments', 'here');

@jasnell Perhaps I can traverse the arguments backwards and see if there is atleast one function object passed?

It's a little verbose, but should be as simple as:

if (typeof mode === 'function') {
  callback = mode;
  mode = fs.F_OK;
}
if (typeof mode !== 'number' || Number.isNaN(mode)) {
  throw new TypeError('mode must be a number');
}
if (typeof callback !== 'function') {
  throw new TypeError('callback must be a function');
}

If we are doing this, we should probably do the same for other methods as well.

okay. I'd like that change, but let's keep that for another PR.

I agree with @jasnell's opinion on grabbing the last argument. Other than that LGTM.

Still LGTM. With the note above that changing how arguments are parsed should go into another PR.

The "fs" module has two functions called `maybeCallback` and `makeCallback`, as of now. The `maybeCallback` creates a default function to report errors, if the parameter passed is not a function object. Basically, if the callback is omitted in some cases, this function is used to create a default callback function. The `makeCallback`, OTOH, creates a default function only if the parameter passed is `undefined`, and if it is not a function object it will throw an `Error`. This patch removes the `maybeCallback` function and makes the callback function argument mandatory for all the async functions.

Rebased. CI Run: https://ci.nodejs.org/job/node-test-pull-request/3366/

Landed in 9359de9.

The "fs" module has two functions called `maybeCallback` and `makeCallback`, as of now. The `maybeCallback` creates a default function to report errors, if the parameter passed is not a function object. Basically, if the callback is omitted in some cases, this function is used to create a default callback function. The `makeCallback`, OTOH, creates a default function only if the parameter passed is `undefined`, and if it is not a function object it will throw an `Error`. This patch removes the `maybeCallback` function and makes the callback function argument mandatory for all the async functions. PR-URL: #7168 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

oh hey all... looks like this broke npm set which in turn is breaking citgm on master

/cc @nodejs/npm

This reverts commit 9359de9. Original Commit Message: The "fs" module has two functions called `maybeCallback` and `makeCallback`, as of now. The `maybeCallback` creates a default function to report errors, if the parameter passed is not a function object. Basically, if the callback is omitted in some cases, this function is used to create a default callback function. The `makeCallback`, OTOH, creates a default function only if the parameter passed is `undefined`, and if it is not a function object it will throw an `Error`. This patch removes the `maybeCallback` function and makes the callback function argument mandatory for all the async functions. PR-URL: nodejs#7168 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

oh hey all... looks like this broke npm set which in turn is breaking citgm on master

Indeed, see npm/npm#13457 for further breakage that this causes in npm. What's the status on this?

Status is that we will be reverting this specific change and taking a
slightly different approach. Long term, callbacks will be required. Near
term it's possible that we'll emit warnings.

On Wednesday, July 27, 2016, Forrest L Norvell notifications@github.com
wrote:

oh hey all... looks like this broke npm set which in turn is breaking
citgm on master

Indeed, see npm/npm#13457 npm/npm#13457 for
further breakage that this causes in npm. What's the status on this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7168 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eVGeyvMobe1tOxQ_py_DFtGXWHB-ks5qZ-xwgaJpZM4IudvB
.

This reverts commit 9359de9. Original Commit Message: The "fs" module has two functions called `maybeCallback` and `makeCallback`, as of now. The `maybeCallback` creates a default function to report errors, if the parameter passed is not a function object. Basically, if the callback is omitted in some cases, this function is used to create a default callback function. The `makeCallback`, OTOH, creates a default function only if the parameter passed is `undefined`, and if it is not a function object it will throw an `Error`. This patch removes the `maybeCallback` function and makes the callback function argument mandatory for all the async functions. PR-URL: nodejs#7168 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

This reverts commit 9359de9. Original Commit Message: The "fs" module has two functions called `maybeCallback` and `makeCallback`, as of now. The `maybeCallback` creates a default function to report errors, if the parameter passed is not a function object. Basically, if the callback is omitted in some cases, this function is used to create a default callback function. The `makeCallback`, OTOH, creates a default function only if the parameter passed is `undefined`, and if it is not a function object it will throw an `Error`. This patch removes the `maybeCallback` function and makes the callback function argument mandatory for all the async functions. PR-URL: #7168 Reviewed-By: Trevor Norris <trev.norris@gmail.com> PR-URL: #7846 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

thefourtheye added 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. labels Jun 5, 2016

ronkorving reviewed Jun 14, 2016
View reviewed changes

thefourtheye force-pushed the remove-maybeCallBack branch from 71adf44 to 2c32c1f Compare June 26, 2016 06:37

thefourtheye force-pushed the remove-maybeCallBack branch from 2c32c1f to bc57b2f Compare June 28, 2016 07:29

jasnell reviewed Jun 28, 2016
View reviewed changes

thefourtheye force-pushed the remove-maybeCallBack branch from bc57b2f to bff78cd Compare July 21, 2016 04:50

thefourtheye closed this Jul 21, 2016

thefourtheye deleted the remove-maybeCallBack branch July 21, 2016 21:06

thefourtheye mentioned this pull request Jul 21, 2016

fs: do basic arg validation of truncate in js #2498

Closed

Trott mentioned this pull request Jul 23, 2016

Role of CTC in semver-major changes needs clarification #7848

Closed

gibfahn mentioned this pull request Jul 25, 2016

[WIP] Update test-npm target #7867

Closed

4 tasks

cjihrig mentioned this pull request Aug 10, 2016

Revert fs changes #7846

Closed

2 tasks

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

fs: remove maybeCallback function by thefourtheye · Pull Request #7168 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: remove maybeCallback function - #7168

fs: remove maybeCallback function#7168thefourtheye wants to merge 1 commit intonodejs:masternodejs/node:masterfrom thefourtheye:remove-maybeCallBackthefourtheye/io.js:remove-maybeCallBackCopy head branch name to clipboard

Conversation

thefourtheye commented Jun 5, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

thefourtheye commented Jun 5, 2016

Uh oh!

benjamingr commented Jun 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Jun 6, 2016

Uh oh!

trevnorris commented Jun 6, 2016

Uh oh!

benjamingr commented Jun 6, 2016

Uh oh!

reqshark commented Jun 6, 2016

Uh oh!

benjamingr commented Jun 6, 2016

Uh oh!

reqshark commented Jun 6, 2016

Uh oh!

trevnorris commented Jun 13, 2016

Uh oh!

ronkorving Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 20, 2016

Uh oh!

Fishrock123 commented Jun 24, 2016

Uh oh!

thefourtheye commented Jun 26, 2016

Uh oh!

trevnorris commented Jun 27, 2016

Uh oh!

thefourtheye commented Jun 28, 2016

Uh oh!

jasnell Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris Jul 20, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Jun 28, 2016

Uh oh!

trevnorris commented Jul 20, 2016

Uh oh!

thefourtheye commented Jul 21, 2016

Uh oh!

thefourtheye commented Jul 21, 2016

Uh oh!

MylesBorins commented Jul 22, 2016

Uh oh!

othiym23 commented Jul 27, 2016

Uh oh!

jasnell commented Jul 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

fs: remove maybeCallback function#7168
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:remove-maybeCallBack

benjamingr commented Jun 5, 2016 •
edited
Loading

jasnell Jun 28, 2016 •
edited
Loading