Callback Style Preference

[Daniel Rinehart]

Is there consensus on the value to pass to a callback if there isn't
an error and there may or may not be a resulting value. A quick survey
of node.js code along with popular modules seems to indicate that null
is most common but undefined and false also pop up frequently. While
any falsey value works I was curious if one was preferred.

-- Daniel R. <dan...@neophi.com> [http://danielr.neophi.com/]

[Mariusz Nowak]

It should be null or undefined only, everything else is a value, and can mistakenly accepted as an error.

There are however asynchronous functions that never resolve with an error (e.g. path.exists) and as there's no point in always sending null to their callbacks, they're designed to pass success value as first argument (in case of path.exists true or false).

-- 

Mariusz Nowak - @medikoo

[Tim Caswell]

The most correct value for the error parameter is "undefined", but most of us are lazy and type "null" instead.  In most code it expects the err parameter to be either falsy or an Error instance, so it doesn't matter if you use false, undefined, null, 0, or even "".

While there are some functions that never emit an error, if there is any chance they would emit one in the future, save yourself the trouble and reserve the err parameter.  Also it makes the function work with flow-control tools and wrappers that assume the pattern.

--
Job Board: http://jobs.nodejs.org/
Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
You received this message because you are subscribed to the Google
Groups "nodejs" group.
To post to this group, send email to nod...@googlegroups.com
To unsubscribe from this group, send email to
nodejs+un...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/nodejs?hl=en?hl=en

[Isaac Schlueter]

path.exists (moved to fs.exists) is an abomination for so many
reasons. We can't get rid of it, because it's too widely used, but
it's really pretty awful. It predates the cb(er, data) pattern in
node, and is one of the very few exceptions to it.

Any time you find yourself using it, you should try to figure out what
you're really trying to find out, and use some other fs call instead.
Usually the best approach is to just try to do what you're going to
do, and handle the error. If you're using a file's existence as a
lock of some sort, it's better to open the file in O_EXCL mode
instead.

[Marco Rogers]

I don't know why we can't change it. Sure it may break a lot of working code. But as you've outlined, replacing it with more preferable methods is fairly straight forward. It's even easily shimmable for folks who don't want to upgrade. We're talking about breaking nextTick but "can't" get rid of this? Doesn't seem logical.

:Marco

>> nodejs+unsubscribe@googlegroups.com

>> For more options, visit this group at
>> http://groups.google.com/group/nodejs?hl=en?hl=en
>
>
> --
> Job Board: http://jobs.nodejs.org/
> Posting guidelines:
> https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
> You received this message because you are subscribed to the Google
> Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com
> To unsubscribe from this group, send email to

> nodejs+unsubscribe@googlegroups.com

[Mark Hahn]

I would be very unhappy if someone broke my code.  Ask the jquery bdfl what happened when he tried to make a non-backwards-compatible change.

[Marco Rogers]

If you're using a pre-1.0 platform, your code is subject to breakage. Also you have to opt into upgrading and every major node update has required minor changes.

At some point we went from changing the node api a lot to make it better to not changing it at all for fear of crying developers. I'm not suggesting we open flood gates. I'm suggesting we continue to refine the api and get as close to one with no unnecessary warts as we can before going 1.0. Because then we really are screwed.

:Marco

[Mark Hahn]

The ver 1.0 moniker is pretty much meaningless.  Many many people have it in production.

--
Job Board: http://jobs.nodejs.org/
Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
You received this message because you are subscribed to the Google
Groups "nodejs" group.
To post to this group, send email to nod...@googlegroups.com
To unsubscribe from this group, send email to

nodejs+un...@googlegroups.com

[Mark Hahn]

It's like google's "beta" gmail was.

[Marco Rogers]

Yes I know that. But that is despite the pre-1.0 version. Node has tried to be very strict about what he version means from the beginning. pre-1.0 means "stable, but not done". Some people think "stable" means "we won't break backwards compat". But I'm suggesting that it actually mean "we'll try really hard for backwards compat, but we might break things for the good of the api". I think in the case of path.exists, this is very doable. If we can't do stuff like this now, then we are resigning ourselves to being stuck with these warts forever. And path.exists/fs.exists is a particular bad wart because it's the exact place people will look for this functionality. So the propagation of this will continue to go up.

:Marco

nodejs+unsubscribe@googlegroups.com

[Mark Hahn]

I understand.  I'm just being contrary.  

>   Node has tried to be very strict about what he version means from the beginning. pre-1.0 means "stable, but not done".  

God forbid it should ever be "done", whatever that means.   I doubt there will be much difference between v 0.999 and v 1.001.

On a more serious note, what harm does .exists do?  http.createServer violates all the rules and it is the first thing a node newbie sees.

nodejs+un...@googlegroups.com

[P. Douglas Reeder]

FWIW, I found path.exists() to be odd when I was first learning Node, and didn't understand how things worked.

Given that there's a straightforward replacement, I see no reason it can't be removed as part of a major update.

[Marco Rogers]

On a more serious note, what harm does .exists do?  http.createServer violates all the rules and it is the first thing a node newbie sees.

Curious. What rules you think createServer violates? 

[Mariusz Nowak]

+1

I looked at the Node's source, and indeed path.exists (to be fs.exists) is just wrapper over fs.stat.

If there's no "lighter" way for checking whether path exists, indeed it's best it it's not in the API at all. There are already two good reasons for that: it encourages bad practices and it's asynchronous function that doesn't follow convention.

[Oliver Leics]

The callback gets no error argument as the first argument.


I would rigorously throw away _all_ of those inconsistencies right
now, but at least with 0.8. For me, the leading zero in a
version-number means: Expect changes, anytime, everywhere.

Nobody could foresee that it is generally best practice to always
reserve the first argument of an callback for error-objects, no matter
if the function checks for file-existence or if it creates a
http-server. But everyone decided to run node.js in production before
the first major release, version 1.0.0. Everyone should know about
that and don't wine if anything breaks.

Change it before 1. But I doubt that this will happen.



BTW: That google-mail 'beta' label was pure marketing.

[Mariusz Nowak]

@Oliver createServer is not asynchronous function, it is synchronous function that returns event emitter and optionally accepts listener. It's different concept/pattern and all rules are followed

[Oliver Leics]

On Thu, May 31, 2012 at 9:15 PM, Mariusz Nowak <mar...@medikoo.com> wrote:
> @Oliver createServer is not asynchronous function, it is synchronous
> function that returns event emitter and optionally accepts listener. It's
> different concept/pattern and all rules are followed

I know that, but the missing error parameter for the callback is still
an inconsistency (even if there are very very rare till zero cases
that actually give an error-object on req-res connection level)

[Mariusz Nowak]

@Oliver Synchronous functions throw errors if there's any, they do not take any callback for that.

[Oliver Leics]

On Thu, May 31, 2012 at 9:26 PM, Mariusz Nowak <mar...@medikoo.com> wrote:
> @Oliver Synchronous functions throw errors if there's any, they do not take
> any callback for that.

You can repeat that "it's a synchronous function" argument as long as
you want, for me it is an inconsistency that the callback of
http.createServer gets no error argument (even if rarely or never
used)

[Oliver Leics]

BTW: http.createServer should/must continue to emit the error event.
That's very consistent.

--
Oliver Leics @ G+
https://plus.google.com/112912441146721682527

[Scott González]

On Thu, May 31, 2012 at 3:39 PM, Oliver Leics <oliver...@gmail.com> wrote:

You can repeat that "it's a synchronous function" argument as long as
you want, for me it is an inconsistency that the callback of
http.createServer gets no error argument (even if rarely or never
used)

The point is that it's not a callback. It's a shortcut for adding a request event listener.

https://github.com/joyent/node/blob/f721d02c8a356c1a9a40d8bf7099186749f01978/lib/http.js#L1607

[Oliver Leics]

On Thu, May 31, 2012 at 9:44 PM, Scott González
<scott.g...@gmail.com> wrote:
> On Thu, May 31, 2012 at 3:39 PM, Oliver Leics <oliver...@gmail.com>
> wrote:
>>
>> You can repeat that "it's a synchronous function" argument as long as
>> you want, for me it is an inconsistency that the callback of
>> http.createServer gets no error argument (even if rarely or never
>> used)
>
>
> The point is that it's not a callback. It's a shortcut for adding a request
> event listener.

Yes, I know that too. And you might have already guessed it: It's just
another inconsistency that confuses ppl ;-)

[Mark Hahn]

>  The point is that it's not a callback. It's a shortcut for adding a request event listener. 

And that is not a common thing in node.  It is just there to make the code sample on the front page look good.

[Mark Hahn]

>   It is just there to make the code sample on the front page look good. 

Which confused me to no end when I first studied node.  I'm sure it confuses many others.  It is a bit of "magic" and magic is not a good thing.

[Jake Verbaten]

Yes, I know that too. And you might have already guessed it: It's just

another inconsistency that confuses ppl ;-)

It's not inconsistent. The server is an event emitter the error handling pattern is server.on("error", errorHandler). There are two error handling patterns.

The other error handling pattern of (err, data) is for asynchronous operations that are not event emitters.

One can argue that the macro of having http.createServer(f) be http.createServer().on("request", f) is confusing 

[Mark Hahn]

>  One can argue that the macro of having http.createServer(f) be http.createServer().on("request", f) is confusing  

They are both confusing to a newbie.  But one of them presents reality and starts the newbie on the road to understanding.  The other is undocumented magic that causes more confusion.

What better time to introduce events?

[Oliver Leics]

On Fri, Jun 1, 2012 at 12:44 AM, Jake Verbaten <ray...@gmail.com> wrote:
>> Yes, I know that too. And you might have already guessed it: It's just
>> another inconsistency that confuses ppl ;-)
>
>
> It's not inconsistent. The server is an event emitter the error handling
> pattern is server.on("error", errorHandler). There are two error handling
> patterns.

Just for the record: I did not say that the events the http-server
emits are inconsistent.

> The other error handling pattern of (err, data) is for asynchronous
> operations that are not event emitters.

I know that.

> One can argue that the macro of having http.createServer(f) be
> http.createServer().on("request", f) is confusing

I did say, that - what you call a macro - is inconsistent, thus
confusing people.

[Isaac Schlueter]

Node is not optimized for "newcomers are not surprised." That's an
occasional side effect of what it IS optimized for: "javascript
programmers can do a lot easily".

You spend very little time as a newcomer. You spend a lot more time
in the state of "I know how X and Y work, and so expect Z to behave
the same." It's more important for APIs to be consistent than
immediately intelligible to someone who has never seen them before.

There are several cases where we do `var thing =
module.createThing(args, someEventHandler)`.

var request = http.get(options, function (response) { })
// vs
var request = http.get(options).on('response', function (response) { })

var watcher = fs.watch(filename, function (change) { })
// vs
var watcher = fs.watch(filename).on('change', function (change) { })

Almost every EventEmitter subclass in node core has some factor
creator that lets you attach a listener to its most relevant event
when creating it. This is a very consistent pattern. Creating a
server or request is not a one-off "I have a question, give me an
answer". It's a long-lived object. fs.exists(cb) violates the
pattern because it is a one-off question, but does not take an error
argument. Furthermore, the failure of stat does not necessarily mean
that the file doesn't exist, so it's also

The only time we don't do this, actually, is when the object doesn't
have a single clear event that you care about. For example, with
child_process.spawn, there are several events that you might be
interested in, and so we don't add the sugar API to add an event
listener at creation time.

Making things needlessly verbose may make newcomers spend a few
minutes less reading documentation the first time, but they'll make us
all spend a few minutes more every time for the rest of our lives.

That's not a good trade.

That's not to say that we don't care about the newcomer experience, of
course. But when designing APIs, it's important not to skew your
least-surprise too far in the direction of never surprising anyone
ever. Newcomers will be surprised by many reasonable things. The
litmus test is if it surprises someone who is familiar with the rest
of the Node.js API. That is how we evolve these things in context.

[Oliver Leics]

On Fri, Jun 1, 2012 at 1:33 AM, Isaac Schlueter <i...@izs.me> wrote:
> var request = http.get(options, function (response) { })
> // vs
> var request = http.get(options).on('response', function (response) { })
>
> var watcher = fs.watch(filename, function (change) { })
> // vs
> var watcher = fs.watch(filename).on('change', function (change) {  })
>
> Almost every EventEmitter subclass in node core has some factor
> creator that lets you attach a listener to its most relevant event
> when creating it.  This is a very consistent pattern.

Why is this? Whats the purpose of that -yes- consistent pattern?

[Isaac Schlueter]

On Thu, May 31, 2012 at 4:45 PM, Oliver Leics <oliver...@gmail.com> wrote:
> Why is this? Whats the purpose of that -yes- consistent pattern?

It makes it easier to build fast, scalable network applications.

[Alan Gutierrez]

On 5/31/12 3:52 PM, Oliver Leics wrote:
> On Thu, May 31, 2012 at 9:44 PM, Scott Gonz�lez

Never confused me. The (err, result) signature means one thing. This
means another. Not very confusing at all. Note: I'm people.

--
Alan - @bigeasy

[Oliver Leics]

On Fri, Jun 1, 2012 at 7:50 AM, Alan Gutierrez <al...@prettyrobots.com> wrote:
> Never confused me. The (err, result) signature means one thing. This means
> another. Not very confusing at all. Note: I'm people.

Good for you :) BTW: You are not people, you are you.

[Oliver Leics]

Ok, from now on I'll call it a macro too and continue to use it.

[Alan Gutierrez]

I believe you're making a talisman of "consistency", where you're
choosing an arbitrary construct in the text, function signatures of
functions passed to API functions, and turning them into a fetish.

If I can get all the chairs in the office to point in the same
direction, the office will be more organized.

Taking an arbitrary aspect of the API and making it fixed will sew
confusion for people when the fixture does not accurately express the
purpose of the callback.

The purpose of the callback to create a server is to register an event
handler for zero, one or more request events. The purpose of the
callback to `fs.readFile` is to respond to a single event, guaranteed to
occur once and only once.

Adding the error as the first argument to the callback for the server's
request handler forces the server to place a block of error handing code
at the head of the request handling function, when only a subset of the
total set of server errors will occur just prior to executing the event
handler.

What errors actually get passed to this server handler? Can you answer
that question in a way that is less confusing to the venerated newbie
than simply saying, "oh, no, this is different from the callbacks your
learned about in `fs`, error handling is a separate event?"

--
Alan Gutierrez - @bigeasy

[Oliver Leics]

tl;dr

> --
> Job Board: http://jobs.nodejs.org/
> Posting guidelines:
> https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
> You received this message because you are subscribed to the Google
> Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com
> To unsubscribe from this group, send email to
> nodejs+un...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/nodejs?hl=en?hl=en

[Bruno Jouhier]

Oliver,

Node has two very different API styles:

1) The callback style where the callback is called exactly once and receives (err, result) arguments
2) The event style where the callback may be called multiple times with event data and where errors are dispatched as a separate "error" event. The event handlers take varying number of arguments (usually one).

Looks like you want to align 2) on 1) by adding an error parameter to event handlers. Sounds wrong as errors already have their own event and this would mean extra code in every event handler to check for errors.

fs.exists is an anomaly because it is a style-1 callback API (callback invoked exactly once) with a non-standard callback signature.

Bruno

On Friday, June 1, 2012 9:03:04 PM UTC+2, Oliver Leics wrote:

tl;dr

On Fri, Jun 1, 2012 at 8:41 PM, Alan Gutierrez <al...@prettyrobots.com> wrote:
> On 6/1/12 3:16 AM, Oliver Leics wrote:
>>

>> On Fri, Jun 1, 2012 at 7:50 AM, Alan Gutierrez<alan@prettyrobots.com>

> nodejs+unsubscribe@googlegroups.com

[Oliver Leics]

Bruno,

On Fri, Jun 1, 2012 at 11:35 PM, Bruno Jouhier <bjou...@gmail.com> wrote:
> Node has two very different API styles:

> [...]

I know that. I knew before I wrote into this thread.

> Looks like you want to align 2) on 1) by adding an error parameter to event
> handlers. Sounds wrong as errors already have their own event and this would
> mean extra code in every event handler to check for errors.

Nope. What I wanted was to express how I think about it and get the
experience how everyone around will communicate about that.

And that was my (non sarcastic) conclusion ;-)

[Scott González]

On Wed, May 30, 2012 at 6:45 PM, Mark Hahn <ma...@hahnca.com> wrote:

I would be very unhappy if someone broke my code.  Ask the jquery bdfl what happened when he tried to make a non-backwards-compatible change.

I tried to let this go, but I just couldn't. jQuery has a history of making non-backwards-compatible changes. Some users update their code, some users stay on old versions, some users patch over it, some users yell (these groups are not mutually exclusive). The library improves, time passes, and things settle down. Our usage and adoption rate is generally unchanged even when we introduce breaking changes. Expect more non-backwards-compatible changes to jQuery in the future.

Node should fix its problems, even if that means a slightly painful upgrade in a specific version for some percentage of users.

[Mark Hahn]

>  . jQuery has a history of making non-backwards-compatible changes. 

I have never been hit by one. Can you  give an example?  I'm not referring to changes that relate to undocumented behavior.

I was referring to a blog entry by resig where he was bitching that he tried to make an incompatible change and the community revolted and he had to undo the change.  I can find it for you if you wish.

I have no problem with deprecating.  That is the friendly way to handle these things.  Causing pain among the base is not the way keep them happy.

[Scott González]

On Mon, Jun 4, 2012 at 4:48 PM, Mark Hahn <ma...@hahnca.com> wrote:

>  . jQuery has a history of making non-backwards-compatible changes. 

I have never been hit by one. Can you  give an example?  I'm not referring to changes that relate to undocumented behavior.

attribute selectors no longer support @, .clone( false ) was dropped, .add() now sorts in DOM order, .attr() only looks at attributes not properties, .val() behavior changed for radio/checkbox, etc. The list goes on.
 

I was referring to a blog entry by resig where he was bitching that he tried to make an incompatible change and the community revolted and he had to undo the change.  I can find it for you if you wish.

You're probably referring to .attr(). The result was not backing anything out, but instead adding a shim for specific common properties (specifically boolean attributes). This shim is planned to be dropped in the future.
 

I have no problem with deprecating.  That is the friendly way to handle these things.  Causing pain among the base is not the way keep them happy.

Yes, but you'll only be unhappy for a short while. Keeping nextTick() as it is now will cause confusion and bugs for eternity. BTW, this is the response you would actually get from the current jQuery BDFL. If you know you have a mistake and fixing that mistake isn't going to kill all of your users, then you should fix it now. Node is young, the proposed change isn't horrible to deal with, and leaving it will just result in problems continuing into the future.

[Mark Hahn]

I stand corrected.  Thx

--

Job Board: http://jobs.nodejs.org/
Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
You received this message because you are subscribed to the Google
Groups "nodejs" group.
To post to this group, send email to nod...@googlegroups.com
To unsubscribe from this group, send email to

nodejs+un...@googlegroups.com