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

thefourtheye · 2016-06-05T22:31:56Z

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

thefourtheye · 2016-06-05T22:33:35Z

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

benjamingr · 2016-06-05T22:38:26Z

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.

bnoordhuis · 2016-06-06T08:50:02Z

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.

trevnorris · 2016-06-06T08:54:01Z

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.

benjamingr · 2016-06-06T09:09:42Z

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

reqshark · 2016-06-06T09:13:43Z

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

benjamingr · 2016-06-06T10:28:45Z

@reqshark wait, what??

reqshark · 2016-06-06T11:08:55Z

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.

trevnorris · 2016-06-13T22:53:31Z

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.

ronkorving · 2016-06-14T09:40:46Z

Should it not throw with arguments.length being 1?

jasnell · 2016-06-20T16:25:39Z

+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.

Fishrock123 · 2016-06-24T11:45:45Z

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

thefourtheye · 2016-06-26T06:41:35Z

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

trevnorris · 2016-06-27T22:33:21Z

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?

thefourtheye · 2016-06-28T07:31:51Z

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

jasnell · 2016-06-28T15:29:22Z

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(typeofmode==='function'){callback=mode;mode=fs.F_OK;}if(typeofmode!=='number'||Number.isNaN(mode)){thrownewTypeError('mode must be a number');}if(typeofcallback!=='function'){thrownewTypeError('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.

trevnorris · 2016-06-28T21:05:07Z

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

trevnorris · 2016-07-20T19:41:05Z

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.

thefourtheye · 2016-07-21T04:52:53Z

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

thefourtheye · 2016-07-21T21:06:42Z

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>

MylesBorins · 2016-07-22T23:18:51Z

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>

othiym23 · 2016-07-27T23:40:25Z

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?

jasnell · 2016-07-28T01:38:30Z

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

Comment threadtest/fixtures/test-fs-readfile-error.js Outdated
ronkorvingJun 14, 2016
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not throw with arguments.length being 1?

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

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

jasnell reviewed Jun 28, 2016
View reviewed changes

thefourtheye force-pushed the remove-maybeCallBack branch from bc57b2f to bff78cdCompare 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

Uh oh!

Uh oh!

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!

ronkorvingJun 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!

jasnellJun 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!

thefourtheyeJul 13, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorrisJul 13, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheyeJul 13, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorrisJul 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!

10 participants

benjamingr commented Jun 5, 2016•
edited
Loading

jasnellJun 28, 2016•
edited
Loading