0
\$\begingroup\$

The following function is accepting PUT connection to a server API, checks authorization to write and then performs write to a database.

The eslint linting tool complains saying "avoid nesting promises" and "Each then() should return a value or throw". I'm new to promises and I am not sure how to improve my code.

app.put('/api/v0/note/:id', (req, res) => {
  const id = req.params.id;
  const uid = req.user ? req.user.uid : null;
  return user_can_edit_note(uid, id).then(yes => {
    if (yes) {
        return db.collection('notes').doc(id).update({
          title: req.body.title,
          text: req.body.text,
          author_uid: req.user ? req.user.uid : null,
          updated_on: admin.firestore.Timestamp.now()
        }).then(() => {
          return res.json({
            ok: "ok"
          });
        });
    } else {
      return res.status(403).json({
        error: "Permission Denied",
        note_id: id
      });
    }
  }).catch((err) => {
    console.error(err);
    return res.status(500).json({error: String(err)});
  });
});
\$\endgroup\$
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Mar 28 at 11:19
3
\$\begingroup\$
// Nothing fancy, we just wrapped the result in an array
const promise1 = someAsyncOperation()
const promise2 = promise1.then(result => [result])
promise2.then(result => console.log(result)) // [result]

// More concisely
someAsyncOperation()
  .then(result => [result])
  .then(resultInArary => console.log(resultInArray)) // [result]

Whatever you return in then becomes the resolved value of the promise it returns. In this case, when promise1 resolved, we added a then that wrapped the result in an array. The returned value becomes the resolved value of promise2.

Now here's where everyone fails when talking about promises. They miss explaining this one behavior which is vital to the concept of "promise chaining":

const promise1 = asyncOperation1()
const promise2 = promise1.then(result1 => asyncOperation2())
promise2.then(result2 => {
  // This will not be invoked until asyncOperation2()'s promise resolves.
  // Also, result2 is NOT asyncOperation2()'s promise, but its resolved value.

  console.log(result2) // [result]
})

// More concisely
someAsyncOperation()
  .then(result1 => asyncOperation2())
  .then(result2 => {
    // This will not be invoked until asyncOperation2()'s promise resolves.
    // Also, result2 is NOT asyncOperation2()'s promise, but its resolved value.

    console.log(result2) // [result]
  })

When you return a promise in a then, two things happen that's different from returning a "normal value".

  1. It uses the resolved value of asyncOperation2() as the resolved value of promise2, not the promise returned by the function call.
  2. promise2 will not resolve and its callbacks will not be invoked until the promise returned by asyncOperation2() resolves.

By now, you should now see how promise "chaining" is achieved. It's because of this one trick, returning promises in then, that allows subsequent thens to "wait" for (more precisely, not fire callbacks until) the previous promise. You could have code that looks like:

asyncOp1()
  .then(r1 => asyncOp2())
  .then(r2 => asyncOp3())
  .then(r3 => asyncOp4())
  .then(r4 => asyncOp5())
  .then(r5 => console.log('done'))
  .catch(e => console.log('one of the above failed'))

So in your code's case, it would look like this:

class ForbiddenError extends Error {
  constructor(message){
    super(message)
    // Add additional properties
  }
}

app.put('/api/v0/note/:id', (req, res) => {
  const id = req.params.id
  const uid = req.user ? req.user.uid : null

  user_can_edit_note(uid, id)
    .then(yes => {

      // The catch() down below will, uhm... catch this throw.
      if(!yes) throw new ForbiddenError()

      // Return the promise of this operation. The next then() handles it.
      return db.collection('notes').doc(id).update({
        title: req.body.title,
        text: req.body.text,
        author_uid: req.user ? req.user.uid : null,
        updated_on: admin.firestore.Timestamp.now()
      })
    })
    .then(() => {
      // Update completed, respond with ok.

      return res.json({
        ok: "ok"
      });
    })
    .catch((err) => {
      // Any throws not recovered in the sequence will invoke this catch.
      // Use this to evaluate your error. You can extend Error to have it carry
      // custom properties and messages.

      if(err instanceof ForbiddenError){
        return res.status(403).json({
          error: "Permission Denied",
          note_id: id
        })
      } else {
        return res.status(500).json({
          error: "Server error"
        })
      }
    })
})
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.