Is it wrong to ignore implicit return inside arrow function in JavaScript or TypeScript when no return required

Question

Very often I come across code that looks something like this:

ngOnDestroy() {
  this.subscriptions.forEach(subscription => subscription.unsubscribe());
}

My question is if this is, while harmless in this example, a bad practice since the implication here is that there is a return value which is equal to whatever unsubscribe() method returns.

The code above does not have any need to return anything out of the arrow function, yet there is an implicit return there, i.e., the body is translated into return subscription.unsubscribe().

Would it be a better practice to code that function as follows? (note the extra curly braces):

ngOnDestroy() {
  this.subscriptions.forEach(subscription => {
    subscription.unsubscribe();
  });
}

I personally like the new array method :-) – Randy Casburn May 14 '18 at 20:23 — Randy Casburn, May 14 '18 at 20:23

score 8 · Accepted Answer · answered May 14 '18 at 20:21

8

Arrow function reduces code that you write so implicit return helps saving some key strokes :) However, in your 2nd example, even there is a return that is undefined. So it is not bad practice, it is cleaner and shorter.

answered May 14 '18 at 20:21

Zohaib Ijaz

21,926
7
38
60

Estus Flask · Answer 2 · 2018-05-14T23:51:28.217

It is a matter of style, but I would call unintended implicit return a bad one. It takes a bit less to type but gives wrong impression about API in use if a developer who reads the code isn't familiar with it. This is less of a problem for well-known API like forEach. Still, if a function returns a value but it's ignored, this may cause confusion.

Implicit return won't cause runtime issues if returned value is undefined, but in case it's defined, a value may affect the code where a function is being called in such way.

An example is a pitfall I accidentally fell once with AngularJS testing. Testing modules accept module configuration functions with same signatures as production modules (a function annotated for dependency injection that returns no value), but the value is just ignored in production module:

// ok
app.config((foo) => foo.barThatReturnsAValue());

While in tests it results in obscure error:

// Error: [ng:areq] Argument 'fn' is not a function, got Object    
angular.mock.module((foo) => foo.barThatReturnsAValue())

The problem that is specific to TypeScript is that such problems usually aren't detected by type checks:

let foo = () => 1;
let bar = (foo: () => void) => {};

bar(foo); // ok

Ah! Exactly what I was thinking! Anyone else has anything to say about this. I was worried precisely about corner cases like these. The worry is that, once the practice of ALWAYS skipping curly braces is adapted, such weird errors will creep up. — user1902183, May 15 '18 at 17:28

score 2 · Answer 3 · answered Jan 17 '20 at 14:13

For cleaner code and increased readability you may also use the void keyword.

For example: (Note the void keyword after the =>)

ngOnDestroy() {
  this.subscriptions.forEach(subscription => void subscription.unsubscribe());
}

No implicit return here :)

Adam Vigneaux · Answer 4 · 2018-05-14T23:49:02.630

0

In theory, it would be a better practice to follow the second example for functions with return values.

In practice, however, it often does not matter if a callback function has a return value, so the cleaner syntax of the first example is worth the ambiguity.

edit: as Randy points out in the comments, if the function returns undefined, there is no difference between the two approaches. I amended my answer to take this into account.

edited May 14 '18 at 23:49

answered May 14 '18 at 20:22

Adam Vigneaux

163
3
11

1

Not necessarily. If `unsubscribe()` returns `undefined` there is absolutely no difference and the lambda/arrow function is cleaner. – Randy Casburn May 14 '18 at 20:23
@RandyCasburn You are correct, thank you for pointing that out. I updated the answer. – Adam Vigneaux May 14 '18 at 23:49

score 0 · Answer 5 · answered May 14 '18 at 20:25

0

Both of your examples produces return results. If you don't really need any return value. You better use for loops

for(var i=0; i<this.subscriptions.length; i++)
{
  this.subscription[i].unsubscribe();
}

Also for loops are much more efficient than forEach here

answered May 14 '18 at 20:25

Muhammad Usman

10,039
22
39

Or why not just place a return statement thus: `return null`? – Randy Casburn May 14 '18 at 20:33
well, you don't need any return value. Ofcourse returning null would work but It's just unnecessary/not needed. Question is about "good practice" :) – Muhammad Usman May 14 '18 at 20:37
Since this is purely an opinion related question, my point to you was that the for loop is as unnecessary as returning null! – Randy Casburn May 14 '18 at 20:38

Is it wrong to ignore implicit return inside arrow function in JavaScript or TypeScript when no return required

5 Answers5