2

Background

I have a function that makes a request to a server. If the request fails, I want to: 1. log the error 2. run a terminal command 2.1 log if the command failed or succeeded

To achieve this I have the following code:

const createRequest = ( { request, logger, terminal } ) => ( { endpoint, timeout } ) =>
    request.get( endpoint, { timeout } )
        .then( response =>
            logger.info( { event: "Heartbeat request succeeded.", status: response.status } )
        )
        .catch( err =>
            logger.error( { event: "Heartbeat request failed.", err } )
                .then( ( ) => terminal.runAsync( "pm2 restart myAPI" ) )
                .then( ( ) => logger.info( { event: "Restarted API." } ) )
                .catch( err => logger.error( { event: "Failed to restart API.",  err } ) )
        );

Now, there are a few things to notice: - logging is async ( sends info to a remote server ) - running the terminal command is async - making the request is ( obviously ) async

Problem?

The problem I have is that my catch has a Promise inside, which means I have nesting. Now, I am strongly against nesting of promises, so I really want to get rid of that, but I just don't see how.

Question

  1. Is it possible to get rid of nesting promises inside a catch ?
  2. If so how?
Flame_Phoenix
  • 16,489
  • 37
  • 131
  • 266
  • Is there a reason why you don't use async / await syntax? I – k0pernikus Jul 17 '18 at 09:03
  • Why are you against nesting promises? – evolutionxbox Jul 17 '18 at 09:07
  • @k0pernikus I am more of a functional guy. async / await is for imperative fans. – Flame_Phoenix Jul 17 '18 at 09:09
  • @evolutionxbox because promise nesting is an anti-pattern ( http://taoofcode.net/promise-anti-patterns/ ) – Flame_Phoenix Jul 17 '18 at 09:09
  • @Flame_Phoenix async and await is nothing but syntactic sugar. Don't assume that your code is more pure for not using it. (Any async operation most likely is already a sideeffect anyway.) Upside is that by using async await you'll get flattened unnested code right away. – k0pernikus Jul 17 '18 at 09:52
  • https://stackoverflow.com/a/38944300/457268 – k0pernikus Jul 17 '18 at 09:54
  • @k0pernikus I am very well aware of what async/await is and does. I never claimed my code to be more pure for not using it. I do claim that it is more functional and declarative and less imperative, which is my end goal here. – Flame_Phoenix Jul 17 '18 at 10:03
  • @Flame_Phoenix thanks for the article. – evolutionxbox Jul 17 '18 at 10:24
  • 2
    @Flame_Phoenix nesting promises is not an anti-pattern. Some computations will require that promises are sequenced in a guaranteed order, but `Promise.all` runs all promises in parallel. – I'm not sure why you consider `async`/`await` syntax imperative. Please see [this example](https://stackoverflow.com/a/50519124/633183) and [this one](https://stackoverflow.com/a/50121218/633183) – Mulan Jul 17 '18 at 14:36
  • @user633183 If you need to use Promises sequentially, you should chain them together ( using `then` ). No need for async/await. The code you have shown makes horrible use of the async/await. In some cases ( where you return a promise and don't even use await ) you don't need async. There are many reasons why async / await is considered imperative style, but I will leave you a blog to read on the matter: https://medium.com/@gab_montes/is-async-await-a-step-back-to-javascript-95e31263dd31 – Flame_Phoenix Jul 17 '18 at 14:51
  • The taoOfCode article is useful but really needs a large "CAUTION" sign. It is far too doctrinaire on what is anti-pattern and what is pattern. For each "anti-pattern" in the article, there are cases (some common, some rare) where you would correctly choose to do precisely what the author discourages. – Roamer-1888 Jul 17 '18 at 15:55
  • It's not always wise to attempt flattening a promise chain. Nesting can have big advantages, in particular (1) [to benefit from closure](https://stackoverflow.com/a/28250687/3478010) and (2) [to benefit from "insulated catches"](https://stackoverflow.com/a/32049994/3478010) (as in this question). – Roamer-1888 Jul 17 '18 at 15:55
  • As currently written, there is a reliance on `logger` methods not themselves throwing (or returning a rejected promise). If they do, a wrong path could be taken through the promise chain. Fixing that is pretty simple but to do so neatly will be a challenge. I had a quick hack at it and ended up with a big mess very quickly. – Roamer-1888 Jul 17 '18 at 15:56
  • @Roamer-1888 The logger's can be rejected, and yes I completely agree with all you said. Perhaps you could put that into an answer more detailed? – Flame_Phoenix Jul 17 '18 at 16:17
  • @Flame_Phoenix, yes I could, but it wouldn't be an answer to the question posed. Your question is really more suitable for [Code Review](https://codereview.stackexchange.com/) in which respondents have more freedom to say what they feel with less possibility of attracting down-votes. – Roamer-1888 Jul 17 '18 at 16:39
  • Even with `async/await` it would not stop the nesting since even synchronous code would have nesting for more than 2 paths - it only makes sense. As a side note loggers should never `throw` anyway since they're the ones called on errors - they should gracefully fallback and exponentially retry sending the error and log it locally. – Benjamin Gruenbaum Jul 17 '18 at 20:17

2 Answers2

3

Problem?

The problem I have is that my catch has a Promise inside, which means I have nesting. Now, I am strongly against nesting of promises, so I really want to get rid of that, but I just don't see how.

– Flame_Phoenix

The problem is that you think you have a problem – or maybe that you posted this question on StackOverflow instead of CodeReview. The article you linked shows where you adopt a naive view about nested promises

You get a whole bundle of promises nested in eachother:

loadSomething().then(function(something) {
    loadAnotherthing().then(function(another) {
                    DoSomethingOnThem(something, another);
    });
});

The reason you’ve done this is because you need to do something with the results of both promises, so you can’t chain them since the then() is only passed the result of the previous return.

The real reason you’ve done this is because you don’t know about the Promise.all() method.

– Code Monkey, http://taoofcode.net

No, Promise.all can only sometimes replace nested promises. A simple counter-example – here, one promise's value depends on the the other and so the two must be sequenced

getAuthorByUsername (username) .then (a => getArticlesByAuthorId (a.id))

Nesting promises is not always necessary, but calling it an "anti-pattern" and encouraging people to avoid it before they know the difference is harmful, imo.


statements are not functional

The other linked article shows where you may have been misguided again

Don’t get me wrong, async/await is not the source of all evil in the world. I actually learned to like it after a few months of using it. So, if you feel confortable writing imperative code, learning how to use async/await to manage your asynchronous operations might be a good move.

But if you like promises and you like to learn and apply more and more functional programming principles to your code, you might want to just skip async/await code entirely, stop thinking imperative and move to this new-old paradigm.

– Gabriel Montes

Only this doesn't make any sense. If you look at all of the imperative keywords in JavaScript, you'll notice none of them evaluate to a value. To illustrate what I mean, consider

let total = if (taxIncluded) { total } else { total + (total * tax) }
// SyntaxError: expected expression, got keyword 'if'

Or if we try to use if in the middle of another expression

makeUser (if (person.name.length === 0) { "anonymous" } else { person.name })
// SyntaxError: expected expression, got keyword 'if'

That's because if is a statement and it never evaluates to a value – instead, it can only rely on side effects.

if (person.name.length === 0)
  makeUser ("anonymous") // <-- side effect
else
  makeUser (person.name) // <-- side effect

Below for never evaluates to a value. Instead it relies on side effects to compute sum

let sum = 0
let numbers = [ 1, 2, 3 ]
for (let n of numbers)
  sum = sum + n        // <-- side effect
console.log (sum)      // 6

The same is true for do, while, switch, even return and all of the other imperative keywords – they're all statements and therefore rely upon side effects to compute values.

What evaluates to a value then? Expressions evaluate to a value

1                          // => 1
5 + 5                      // => 10
person.name                // => "bobby"
person.name + person.name  // => "bobbybobby"
toUpper (person.name)      // => "BOBBY"
people .map (p => p.name)  // => [ "bobby", "alice" ]

async and await are not statements

You can assign an asynchronous function to a variable

const f = async x => ...

Or you can pass an asyncrhonous function as an argument

someFunc (async x => ... )

Even if an async function returns nothing, async still guarantees we will receive a Promise value

const f = async () => {}
f () .then (() => console.log ("done"))
// "done"

You can await a value and assign it to a variable

const items = await getItems () // [ ... ]

Or you can await a value in another expression

items .concat (await getMoreItems ()) // [ ... ]

It's because async/await form expressions that they can be used with functional style. If you are trying to learn functional style and avoid async and await, it is only because you've been misguided. If async and await were imperative style only, things like this would never be possible

const asyncUnfold = async (f, initState) =>
  f ( async (value, nextState) => [ value, ...await asyncUnfold (f, nextState) ]
    , async () => []
    , initState
    )

real example

Here's a practical example where we have a database of records and we wish to perform a recursive look-up, or something...

const data =
  { 0 : [ 1, 2, 3 ]
  , 1 : [ 11, 12, 13 ]
  , 2 : [ 21, 22, 23 ]
  , 3 : [ 31, 32, 33 ]
  , 11 : [ 111, 112, 113 ]
  , 33 : [ 333 ]
  , 333 : [ 3333 ]
  }

An asynchronous function Db.getChildren stands between you and your data. How do you query a node and all of its descendants?

const Empty =
  Symbol ()

const traverse = (id) =>
  asyncUnfold
    ( async (next, done, [ id = Empty, ...rest ]) =>
        id === Empty
          ? done ()
          : next (id, [ ...await Db.getChildren (id), ...rest ])
    , [ id ]
    )

traverse (0)
// => Promise [ 0, 1, 11, 111, 112, 113, 12, 13, 2, 21, 22, 23, 3, 31, 32, 33, 333, 3333 ]

A pure program sent from the "heaven of JavaScript developers", to put it in the words of Montes. It's written using a functional expression, errors bubble up accordingly, and we didn't even have to touch .then.

We could write the same program using imperative style. Or we could write it functional style using .then too. We can write it all sorts of ways and I guess that's the point – Thanks to async and await's ability to form expressions, we can use them in a variety of styles, including functional style.

Run the entire program in your browser below

const asyncUnfold = async (f, init) =>
  f ( async (x, acc) => [ x, ...await asyncUnfold (f, acc) ]
    , async () => []
    , init
    )

const Db =
  { getChildren : (id) =>
      new Promise (r => setTimeout (r, 100, data [id] || []))
  }

const Empty =
  Symbol ()

const traverse = (id) =>
  asyncUnfold
    ( async (next, done, [ id = Empty, ...rest ]) =>
        id === Empty
          ? done ()
          : next (id, [ ...await Db.getChildren (id), ...rest ])
    , [ id ]
    )
    
const data =
  { 0 : [ 1, 2, 3 ]
  , 1 : [ 11, 12, 13 ]
  , 2 : [ 21, 22, 23 ]
  , 3 : [ 31, 32, 33 ]
  , 11 : [ 111, 112, 113 ]
  , 33 : [ 333 ]
  , 333 : [ 3333 ]
  }

traverse (0) .then (console.log, console.error)
// => Promise
// ~2 seconds later
// [ 0, 1, 11, 111, 112, 113, 12, 13, 2, 21, 22, 23, 3, 31, 32, 33, 333, 3333 ]
Mulan
  • 129,518
  • 31
  • 228
  • 259
-1

I think there are two ways.

Option 1

Keep using Promise, but the code need some changes:

const createRequest = ({ request, logger, terminal }) => ({ endpoint, timeout }) =>
  request.get(endpoint, { timeout })
    .then(response =>
      logger.info({ event: "Heartbeat request succeeded.", status: response.status })
    )
    .catch(err => {
      // after a catch the chain is restored
      return logger.error({ event: "Heartbeat request failed.", err })
    })
    .then(() => terminal.runAsync("pm2 restart myAPI"))
    .then(() => logger.info({ event: "Restarted API." }))
    .catch(err => logger.error({ event: "Failed to restart API.", err }))

Option 2

Use await/async, change async to sync.

Chengyzh
  • 1
  • 2
  • what will happen if the request succeeds ? ( It's s tip, your code is incorrect :P ). Async / await is out of the option. – Flame_Phoenix Jul 17 '18 at 09:41
  • @evolutionxbox When is for predicate conditions. What would be the predicate here? I believe the right function would be a try/catch, as ironic as it may be. Still nice suggestion! – Flame_Phoenix Jul 17 '18 at 10:30
  • Your logging does nesting - even if it was completely synchronous you'd have nesting... – Benjamin Gruenbaum Jul 17 '18 at 20:15