Very often I e 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();
  });
}

Very often I e 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();
  });
}

• javascript
• typescript

• I personally like the new array method :-) – Randy Casburn Commented May 14, 2018 at 20:23
• Haha. Corrected. – user1902183 Commented May 15, 2018 at 0:12

5 Answers 5

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.

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

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 :)

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

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 ments, if the function returns undefined, there is no difference between the two approaches. I amended my answer to take this into account.