fs: Accept a function as an options container.

[ChALkeR]

This is alternate to #1982

This should fix a breaking change in commit 353e26e, that was included in a minor version of io.js and broke at least two modules (that were misusing fs.createWriteStream method, but still).

Those two modules were passing a function to fs.createWriteStream, and that was never supported (it was always ignored). In 2.3.0 a more strict options validation was introduced, it started throwing an Error, and at least those two modules were broken.

This patch uses a simple way to get around that, treating a function as a valid options container.

Apart from functions, 353e26e changed the behaviour on falsy variables (null and 0 started causing an Error), and that is not addressed in this PR. I could fix that, replacing the options === undefined with a !options check (that would involve changing the expectations of several tests). What do you think?

@yosuke-furukawa, @chrisdickinson

Fixes: #1981

[yosuke-furukawa]

TL;DR

LGTM

my opinion

We should consider what API is really useful. If function call is not satisfied the required condition, we need to raise an alert like your function has a bug.

For example, if an user wants to use fs.writeFile('path.txt', function(err){}) actually, but the user accidentally uses fs.createWriteStream('path.txt', function(err){}) instead.
The API needs to raise error and give the user message like This API usage is mistaken.

So, I think this current API is correct.

ideal solution

When I was in the similar situation, I research the usage and consider what is correct and really useful API, then I sent the PR #1412 .

Of course, I could revert the PR #635 .
However I decided we use the 2nd argument as an encoding type if the 2nd argument is string.

The PR has my decision:

BUT I have 2 opinions.
the error message(TypeError: Object prototype may only be an Object or null: utf8) is not so easy to understand. We should return Bad Arguments like other functions.
fs.createReadStream and fs.createWriteStream should accept 2nd string argument as an encoding type. our existing functions like fs.readFile, fs.writeFile, have same behavior like here. the behavior misleads some developers.

If some users really want to use callback function in fs.createWriteStream, we should know the use-case firstly. If callback is really useful, we should allow the callback. BUT if fs.createWriteStream is misused instead of fs.writeFile, we need not to fix the situation, those modules should fix the bug.

realistic

Of course, the above opinion and solution is ideal. I don't think this is pretty good idea.
In real world, we should not break the modules on minor version up under any reasons.

[ChALkeR]

@yosuke-furukawa, What do you think about a falsy second argument?

[targos]

I think that if some people are misusing fs.createWriteStream with a callback, we need to teach them that it is incorrect, maybe by adding something about it in the docs.
It would be friendly to add support for callback but not a good thing IMO. The streams API is different from the errback one and users should know that. It would be misleading to introduce in core a method that mixes both APIs.

[targos]

(responding to @yosuke-furukawa, I am not against the changes in this PR)

[ChALkeR]

@targos

It would be friendly to add support for callback but not a good thing IMO

I'm -1 on adding proper errbacks support here.

[yosuke-furukawa]

@ChALkeR

What do you think about a falsy second argument?

If we got any issues about a falsy second argument, we should fix that. But currently, we have not got the report. So we don't need to fix falsy argument.

@targos

The streams API is different from the errback one and users should know that. It would be misleading to introduce in core a method that mixes both APIs.

Yes, I agree. I guess a few developers are just misunderstanding the fs.create{Read|Write}Stream API.
Adding a notice in our API docs is good idea.

[bricss]

Don't think that is a good idea to passing function as second argument. It would be good to update docs for fs.createWriteStream and fs.createReadStream and show deprecation message if somebody passing function to arguments.

[ChALkeR]

Ah, @bnoordhuis for the #1981.

And do not get the idea wrong, I am +0 for both this PR and #1982 myself.

I also think that the main issue in on the modules side misusing the API in an undocumented way. But the fact here is that the behaviour was broken in a minor release, and this is close to breaking _prefixed misusing.

TL;DR: I am not the one to advocate for these kind of «fixes» and it would be perfectly fine by me if both of the PRs are rejected. I'm just trying to make it easier to come up with a solution.

[yosuke-furukawa]

@ChALkeR
Thank you. I have a same idea for @bnoordhuis's one.

[ChALkeR]

Ok, as no one (including myself) seems to actually support the idea of handling this (wrong API usage by modules) on the io.js side, I closed these PRs (both #1982 and #1998).
If anyone of @nodejs/collaborators thinks that this or #1982 should be reopened or that there needs to be more discussion on this matter, just leave a message here.

[seishun]

The docs state that options must be a string or an object, and functions are objects, so why not accept functions? I don't see why anyone would want to do this, but neither why it shouldn't be allowed.

I'm +1 on this, not because of buggy modules, but for consistency.

[bricss]

@seishun Yes, we can do this, but this callback will do nothing further as the most of fs method do. So, one possible way to wrap up passed function around with error and close events and call this function after with passed arguments from fired events.

[rlidwka]

The docs state that options must be a string or an object, and functions are objects, so why not accept functions?

Functions are not objects, and using them as option bags could be confusing like hell.

fs.readFileSync('/etc/passwd', function hey_i_am_an_option_object(err, data) {
  console.log(data)
})

This kind of thing is explicitly forbidden in other places, so we should forbid it in ReadStream for consistency reasons.

If you do want to use a function as an option bag, just create an object out of it using Object.create.

[seishun]

Functions are not objects

Yes they are.

using them as option bags could be confusing like hell

Confusing to whom? No one is forcing you to write code like this.

This kind of thing is explicitly forbidden in other places

fs.readFileSync just has an inadequate check for an object. I consider it a bug, since it's not explicitly forbidden in the docs.

[ChALkeR]

Imo, for consistency, in an ideal case, functions should not be treated as option bags.

1. You just can't treat them as option bags everywhere, because it would break existing API. For example, see fs.watch.
2. It's not a good idea to treat them as option bags in some places and not treat them as such in other places.
3. Personal opinion: supporting that would be supporting really bad coding style. We have a lot of places where people could shoot themselves in their feet without this, why should we add an extra one?

The only reason that would justify that would be backwards compatibility with a noticable amount of valid code. But to the moment I found only two modules, and in both cases it was an error on the module side (that code didn't do anything sensible). One of those modules is already fixed, the second one has an open PR to fix that.

[rlidwka]

Yes they are.

That statement only means that Function prototype chain ends with Object, which is irrelevant here. And as far as typeof operator goes, those are completely different entities.

The issue here is that it is impossible to distinguish a function used as a callback and a function used as an option bag. This is why mixing them up is a bad idea.

[seishun]

You just can't treat them as option bags everywhere, because it would break existing API. For example, see fs.watch.

I don't see the problem, it just needs a special case for two arguments: if the second argument is a function, assume listener, otherwise options. There is no ambiguity when called with three arguments.

That statement only means that Function prototype chain ends with Object, which is irrelevant here.

No, it doesn't. "Object type" has nothing to do with prototype chains.

And as far as typeof operator goes, those are completely different entities.

typeof is irrelevant here, since it doesn't return the type. Do you consider null an object?

The issue here is that it is impossible to distinguish a function used as a callback and a function used as an option bag. This is why mixing them up is a bad idea.

It's only an issue when there is ambiguity. I've addressed this in my response to @ChALkeR.

[ChALkeR]

@seishun Please share a valid usecase for the «feature» of using functions as option bags when calling some io.js API method. From what I have seen, 100% of such cases were bugs in users code.

[seishun]

@ChALkeR

I'll quote myself, I don't see why anyone would want to do this, but neither why it shouldn't be allowed.

Either way, if it's disallowed, then the docs must be fixed. Writing "an object that isn't a function" in every function that accepts an options object would be wordy and seem like an arbitrary restriction.

[bricss]

I can not imagine it to myself how to use functions as option bags, it shouldn't be allowed at all.

If somebody wants to use functions as callback it may be possible to add this feature.

[rlidwka]

I don't see the problem, it just needs a special case for two arguments: if the second argument is a function, assume listener, otherwise options.

If we do whatever you're suggesting we do, we'll end up with:

var options1 = { flag: 'r' };
var options2 = function(){};
options2.flag = 'w+';

fs.writeFileSync('/etc/passwd', options1); // this works
fs.writeFile('/etc/passwd', options1); // this works
fs.writeFileSync('/etc/passwd', options2); // this works
fs.writeFile('/etc/passwd', options2); // this doesn't since user "obviously" meant to pass options

Do you see the issue with it?

No, it doesn't. "Object type" has nothing to do with prototype chains.

Hmm... I stand corrected, there is an "object type" in es5 spec. But the spec uses different language than io.js does.

What es5 spec calls "object type", io.js calls "non-primitive type". And "objects" are simply defined as "every non-primitive, typeof of which is object" (you can see it here).

And this kind of a language is so commonly used, so adding "hey functions aren't considered objects" disclaimer is hardly worth it. And one would unlikely use function as an options object deliberately anyway (hey, all cases we found are bugs).

typeof is irrelevant here, since it doesn't return the type. Do you consider null an object?

I wonder what do you think typeof returns if not the type (null is a well-known exception here).

[seishun]

Do you see the issue with it?

Yes. writeFile has a non-optional callback, so this example doesn't work.

But the spec uses different language than io.js does.

And I think it's a problem that io.js is perpetuating a misconception.

(you can see it here)

That was a bug. Since it couldn't be fixed without potentially breaking code, there is now a discussion to deprecate these methods altogether. As a temporary solution, the docs for util.isObject now explicitly state that it "returns true if the given "object" is strictly an Object and not a Function".

I wonder what do you think typeof returns if not the type

It returns a string determined by the type, with a special case for objects, as defined here. It's not the same as returning the type. The way it treats null isn't any more of an exception than the way it treats functions.

[yosuke-furukawa]

OK, I will write some notices in our API doc.
I think we need to write function is not allowed in fs.create{Read|Write}Stream to avoid confusion.
For example https://github.com/nodejs/io.js/pull/1295/files

[rlidwka]

I think we need to write function is not allowed in fs.create{Read|Write}Stream to avoid confusion.

Is there anyone who tries in the real code to pass a function as an options object?

'cause all two cases we found so far are people trying to pass a callback to a function that doesn't accept any. And it doesn't warrant a notice.

[yosuke-furukawa]

Is there anyone who tries in the real code to pass a function as an options object?

I guess not so many (maybe zero). But it is not zero percent that users misunderstand that function is a kind of object, function is acceptable in fs.create{Read|Write}Stream.

Especially, fs.create{Read|Write}Stream has some cases misleading about API...
We should prevent that some users use the callback function in fs.create{Read|Write}Stream.

So I will send a PR to add a notice in fs.create{Read|Write}Stream.

[sam-github]

I used to be of the opinion that any js type that could have properties set on it should be allowed as an options "object". Why restrict the user? You can set properties on functions and strings in js, so why should we tell people not to? Doing so is just a feature of the language, if you don't like it, use another language!

I've come around to agreeing with @rlidwka: its confusing, usually a bug, and in the case of the node APIs, which have a lot of optional intermediate arguments that we have to disambiguate based on type (edit: and functions that behave differently based on type, like listen()), it matters if things are actually an Object, an Array, or a Function. I don't think we should document this in every single API doc paragraph, but I do think we should add a meta statement in the API doc intros stating the intended behaviour of node APIs, and then consider every API that doesn't work as intended a bug, and fix them.

This, for example, is just plain weird:

> net.createServer().listen(function() {}).
  on('listening', function(){console.log(this.address())})
> { address: '::', family: 'IPv6', port: 60485 }

Clearly a bug, I'd say, and yet listen is done on an ephemeral port :-(

/cc @chrisdickinson, because I mentioned docs

[yosuke-furukawa]

We already closed this PR, we should not discuss on the closed PR.

[chrisdickinson]

Hi. Sorry, I've been traveling and unable to check the tracker as often as needed.

Passing a function to fs.createXStream should not throw a TypeError in this version of io.js. If we want to make it do that in the future, that's a different proposition.

for the time being folks having working code written against the idea that these methods take callbacks. That code takes precedence over our desire to "teach" them our API — we should not have broken it in the first place.

Most of the arguments here deal with the inconsistency of supporting a function as a property bag. The issue is that that is how they were dealt with before, and we're restoring that behavior before going through a deprecation path (or adding an API for err-backs). With that in mind, I'd like to re-open this PR.

On Jun 19, 2015, at 2:02 PM, Yosuke Furukawa notifications@github.com wrote:

Is there anyone who tries in the real code to pass a function as an options object?

I guess not so many (maybe zero). But it is not zero percent that users misunderstand that function is a kind of object, function is acceptable in fs.create{Read|Write}Stream.

Especially, fs.create{Read|Write}Stream has some cases misleading about API...
We should prevent that some users use the callback function in fs.create{Read|Write}Stream.

So I will send a PR to add a notice in fs.create{Read|Write}Stream.

—
Reply to this email directly or view it on GitHub.

[ChALkeR]

@chrisdickinson Reopened.
What are your thoughts about falsy arguments? Old versions defaulted those to {}, current versions throw Errors.

[chrisdickinson]

We should follow the old version, and plan to add a deprecation notice for false-y options in v3 (to be removed in v4 if the change doesn't break everything).

On Jun 20, 2015, at 11:09 AM, Сковорода Никита Андреевич notifications@github.com wrote:

@chrisdickinson Reopened.
What are your thoughts about falsy arguments? Old versions defaulted those to {}, current version throws Errors.

—
Reply to this email directly or view it on GitHub.

[yosuke-furukawa]

We should raise this issue to tc-agenda ??
I guess TC member do not have a same opinion . #1981 (comment)

[brendanashworth]

The regression has been around for a month now, perhaps it should be for quick resolution. I'm still in the pot of allowing it with a deprecation message, because upstream users use these modules and shouldn't be responsible for small bugs like this - it was not in a semver-major release and upstream users shouldn't have to worry about this in semver-minor releases.

[Fishrock123]

Make that two months. I would be ok doing it with a deprecation message, if anyone feels strongly about this. :s

Re-open after updated if so.