Question about code style: arrow function V.S. callback function

[XadillaX]

• Version: -
• Platform: -
• Subsystem: -

I want to discuss the code style of arrow function in Node.js source code.

Maybe most of you think that => can replace all of anonymous callback functions. So Node.js source code includes lots of arrow functions. e. g.

// _http_agent.js
nextTick(asyncId, () => ...);

// _http_client.js
req.once('response', (res) => {

etc.

But IMHO, I think arrow function should only use in some situation that the function is just used as a inline lambda expression even though they have a very convenient feature - value of this. That means arrow function should not be misused. And anonymous function should used as callback (mainly asynchronous) or listener functions.

So that's my opinion:

array.map([ ... ], val => { /* do some thing */ });
array.reduce([ ... ], (ans, val) => { ... }, {});

req.on('response', function() {
  // do something
});

fs.readFile(filename, function() {
  // do something
});

I think we can discuss with this and if my opinion is acceptable, I can start working with modifying the style.

[gireeshpunathil]

@XadillaX - thanks. So your proposal is to select between the two based on the number of lines of code they encompass? The bearing the arrow functions have on their enclosure w.r.t receiver, arguments etc. and the necessity for using those bindings in the code should be the deciding factor in choosing one over the other right?

[gireeshpunathil]

On a second thought, if we are migrating from anonymous inliners to arrow functions, I guess there will not be any such thing as this bindings etc. existing. We can refactor the code based on the context, bring in arrow functions based on the opportunity to use enclosure bindings.

[bnoordhuis]

But IMHO, I think arrow function should only use in some situation that the function is just used as a inline lambda expression even though they have a very convenient feature - value of this. That means arrow function should not be misused. And anonymous function should used as callback (mainly asynchronous) or listener functions.

You don't explain why you think that. I can guess (and probably guess right) but it's better if you state it explicitly.

[XadillaX]

In my opinion, in semantics arrow function stands for a inline lambda expression that input some arguments and do some simple logic to output a value.

eg. A map function requires an array and a lambda expression to deal with each element in the array, so we should use arrow function. And most time they should exist in a synchronous context.

[ '', null, 1, undefined, -1 ].filter(v => !!v);
[ 1.1, 2.2, -1.2, -1.3 ].map(v => {
  if(v > 0) return parseInt(v);
  else return v;
});

And if we need a function to stand for a callback or a listener function, I think we should use an anonymous or named function. In that function we will do lots of logic whether it's synchronous or asynchronous and does not necessarily return data like a lambda expression.

eg.

fs.readFile(filename, function(err, data) {
  // do something
  // eg.
  resp.send(data);
});

The most important point for me is 'in semantics'.

I think in semantics arrow function stands for a lambda expression that input some arguments and do some simple logic to output a value.

[XadillaX]

I saw some code where in my opinion ought to be anonymous function that written with arrow function.

e.g.

req.once('response', (res) => {
  // ...
});

So I want to try to discuss about it and see if we can do any standardizing with them.

[XadillaX]

IMHO I consider arrow function is something more like a inline closure or a scope, but not a function. So in semantics it's not suitable to use as a callback function or a listener.

Sorry for my pool English that may not convey my opinion exactly.

[gireeshpunathil]

var ev = require('events').EventEmitter

function foo() {
  this.buf  = new Buffer(1024)
  this.buf.fill('g')
}
foo.prototype.handle = function(e) {
  // e.on('never', () => {this})
  e.on('never', function() {this})
}

var e = new ev()

while (true) {
  new foo().handle(e)
  gc()
}

This code shows the difference in terms of memory rentention - if you swap the normal listener for the arrow function, you see the memory shoots up - as the object foo() is held up with the arrow, while the listener only retains its closure context.

While both leak memory in this scenario, the arrow causes more damage.

So I guess the usage should be based on whether the continuation function needs the receiver obejct in the lexical scope or not - which may not be the case in most of the existing tests.

So a best practice would be:
completion handlers - on a need basis,
listeners - almost never.

Does this sound good?

[mscdex]

@gireeshpunathil Have you posted an issue on the V8 issue tracker about that? I'm not sure there should be such a difference...

[TimothyGu]

@gireeshpunathil Well the thing is, even though a human being can reason and say 'never' is never called, V8 won't know that until e gets out of scope. So, while the arrow function does more damage as you say, it is the expected behavior.

[TimothyGu]

And I'd like to point out the fact that no one would write this if they don't need this, either in function() {} or an arrow function. In fact, common alternatives to arrow functions (var self = this and function() {}.bind(this)) would cause the foo object to not get GC'd either. The arrow is doing its job here just fine.

[evanlucas]

I don't agree that they should only be used for lambda like expressions. I use them pretty heavily for the non-binding of this. I would be -1 to making this change.

[XadillaX]

The point is - should we use it for non-binding of this and then give up the semantics?

That is: convenient V.S. semantics.

[gireeshpunathil]

Looking back, I guesss my point was not clearly articulated. Let me try once again:

1. Does the test case reveal a bug in v8? No. It precisely manifests the specification of arrow function, and nothing more.
2. Is arrow function doing more damage than normal function an expected behavior? Yes. It holds up the enclosure's receiver obejct, exactly what it is supposed to do.
3. Will any one use this if they don't need this? No, not in the manner I have written. This is a simulation program, intended to reveal the external implications with as minimal semantics as possible, but I guess that got mis-fired.
4. Am I advocating against arrow functions? No. The very reason they exist (for binding enclosure's receiver and thereby eliminating the var that = this smuggling) should be leveraged when that is needed.

So in short as to when to use arrow functions, here is my opinion:
a. For completion handlers (one time callback, memory retention is deterministically static): on a need basis.
b. For listeners (recurring callback, memory retention is non-deterministic): Almost never, but use it with caution if you know (and control) the life-cycle of the event the listener is for.

[bnoordhuis]

Is arrow function doing more damage than normal function an expected behavior? Yes. It holds up the enclosure's receiver obejct, exactly what it is supposed to do.

But only because its body refers to this. The enclosing this is captured as needed; i.e., the memory footprint of () => {} and function() {} is the same.

I propose to close the issue because there is no consensus and no technical reason one is better than the other.

[TimothyGu]

I don't think @gireeshpunathil's point was what OP referred to. I believe what @XadillaX was arguing for is semantic correctness of arrow functions' original intent: to be lambda functions in a strictly mathematical sense, rather than just syntactic sugar for .bind(this).

I don't necessarily agree with this. The introduction of () => { statement; } in addition to the shorter () => expression and the possibility for the arrow function to take 0 parameters pretty much signifies the Committee's acceptance of the use of arrow function for purposes more than just lambda functions (in a strict mathematical sense). So I am for closing this ticket as well.

[bnoordhuis]

I'll close this out since there is clearly no consensus.