ESLint consistent-return and nested callbacks

Question

I'm having some issues with the consistent-return rule provided by ESLint. The code below will throw a consistent-return warning for the User.findOne callback function. As far as I can tell, I can only run the .remove action if there is no findErr and if there is an existingUser.

What would be the best practice for avoiding these nested callbacks? I would like to pass the consistent-return warning, but haven't really seen any solutions online.

function remove(req, res) {
  User.findOne({ username: req.params.username }, (findErr, existingUser) => {
    if (findErr) return res.status(500).send(errorHandler.getErrorMessage(findErr));
    if (!existingUser) return res.status(404).send({ message: 'User not found' });

    existingUser.remove((removeErr) => {
      if (removeErr) return res.status(500).send(errorHandler.getErrorMessage(removeErr));

      return res.json({ message: `${existingUser.username} successfully deleted` });
    });
  });
}

You're returning `undefined` if you call `remove`. – Paul Nov 08 '16 at 17:00 — Paul, Nov 08 '16 at 17:00

score 1 · Accepted Answer · answered Nov 08 '16 at 17:02

1

What would be the best practice for avoiding these nested callbacks?

Promises.

I would like to pass the consistent-return warning

Then you'd best not use early-returns. Instead, write

function remove(req, res) {
  User.findOne({ username: req.params.username }, (findErr, existingUser) => {
    if (findErr) res.status(500).send(errorHandler.getErrorMessage(findErr));
    else if (!existingUser) res.status(404).send({ message: 'User not found' });

    else existingUser.remove((removeErr) => {
      if (removeErr) res.status(500).send(errorHandler.getErrorMessage(removeErr));

      else res.json({ message: `${existingUser.username} successfully deleted` });
    });
  });
}

Alternatively, you should be able to do

function remove(req, res) {
  User.findOne({ username: req.params.username }, (findErr, existingUser) => {
    if (findErr) {
      res.status(500).send(errorHandler.getErrorMessage(findErr));
      return;
    }
    if (!existingUser) {
      res.status(404).send({ message: 'User not found' });
      return;
    }
    existingUser.remove((removeErr) => {
      if (removeErr) {
        res.status(500).send(errorHandler.getErrorMessage(removeErr));
        return;
      }
      res.json({ message: `${existingUser.username} successfully deleted` });
    });
  });
}

You also could enable the { "treatUndefinedAsUnspecified": true } option and use return void ….

answered Nov 08 '16 at 17:02

Bergi

630,263
148
957
1,375

1

Let me see if I understand. What you're saying is that the "inconsistent" return statements in my current code comes from the fact that some are `return res.status(404)...` and some are `res.status(500)...` and so on. Whereas in your answer, even though there is no explicit `return` at the end of `User.findOne`, every `return` throughout the entire block is the same....a simple `return;`. Is my understanding correct? Could you expand further on converting this answer to a more best-practice friendly `Promise`? I tried, but didn't manage a good solution. – germainelol Nov 08 '16 at 17:15
The inconsistency is that in the `if` blocks you `return res.…` (which looks like a result value), but in the else case *you don't `return` anything*. Notice that every function (and every callback) is treated separately. Regarding the conversion to promises, that better is answered in [an extra question](http://stackoverflow.com/questions/ask). – Bergi Nov 08 '16 at 17:21
I understand that with your first solution there are no `return`s anywhere, so it's just a simple function that runs some `if`/`else` statements. But in the second solution, isn't it still the case that it returns something in the `if` blocks, and in the `else` situation it doesn't return anything? In the `else` case it's just running `.remove` and not returning anything for `User.findOne`? – germainelol Nov 08 '16 at 17:27
@germainelol `return;` doesn't return anything either, so I guessed it would be fine. I haven't tested against ESlint yet, though. – Bergi Nov 08 '16 at 17:30
It's working with ESLint, was just trying to understand the logic. I think I will take the `Promise` route that you recommended since it's better practice. I assume the correct path would be to `return new Promise` and in the `.catch()`, do something like `reject(httpStatus, message)` to handle the different `res.status` calls I included. – germainelol Nov 08 '16 at 17:34
Also added a promise related question if you would like to help with that as I am stuck with it! http://stackoverflow.com/questions/40493290/converting-a-nested-callback-to-a-promise – germainelol Nov 08 '16 at 17:44

ESLint consistent-return and nested callbacks

1 Answers1