Is this the right use of Promises

Question

I have been exploring Promises for a while in JavaScript and thought of getting better at it.

I made a small blog app in Node. The scenario is whenever a user is deleted I want to delete all the blogs created by the user and also remove the user from the likes of the blog.

Below is the code for it.

        const user = req.user;

        console.log(user);

        // Finding the blogs created by the user and creating an array of promises
        const createdBlogs = await Blog.find({creator: user._id});
        const createdBlogsPromises = createdBlogs.map(async blog => await Blog.findByIdAndDelete(blog._id));
        
        // Finding the blogs liked by the user and creating an array of promises
        // removeUser is an instance method in blogSchema
        const likedBlogs = await Blog.find({likes: {$in: user._id}});
        const likedBlogsPromises = likedBlogs.map(async blog => await blog.removeUser(user));

        // Using all method for parallel execution of promises
        const promiseResponse = await Promise.all([...createdBlogsPromises, ...likedBlogsPromises]);

        console.log(promiseResponse);
        
        await User.deleteOne({_id: user._id});

        res.status(204).json({message: 'Successfully deleted!!'});

I am using Promise.all() for this. Is this the right way to use Promise.all() or is there an alternative and easy way to perform this ?

score 2 · Answer 1 · answered Jul 16 '23 at 17:20

This is about right, but there are two problems:

Instead of calling Blog.findByIdAndDelete(blog._id) and blog.removeUser(user) (which I supposed actually does remove the likes?) many times, the blog repository service should ideally have methods like Blog.deleteAll(blogIds) where you can pass an array of elements to operate on. This would be significantly faster if supported by the underlying storage layer (database?) as a single command.

You are constructing the createdBlogsPromises, but then before waiting for them you first do another await Blog.find(…). If there's an error in both of those, only the awaited promise rejection can be handled by the caller (e.g. with try/catch), but the other error will result in an unhandled promise rejection that crashes your server. See Waiting for more than one concurrent await operation and Any difference between await Promise.all() and multiple await? for details.
Notice it's not the Promsie.all that "executes" the promises (as indicated in your comment), the asynchronous code starts running when you call it. Promises are just a mechanism to wait for the results.

You can fix this by immediately awaiting the array of promises:

const createdBlogs = await Blog.find({creator: user._id});
await Promise.all(createdBlogs.map(blog => Blog.findByIdAndDelete(blog._id)));

const likedBlogs = await Blog.find({likes: {$in: user._id}});
await Promise.all(likedBlogs.map(blog => blog.removeUser(user)));

await User.deleteOne({_id: user._id});

However, this means everything runs sequentially now, which you didn't intend. (Also, even in your original code createdBlogs and likedBlogs were found sequentially, which is not necessary). You can refactor this for full concurrency as

async function deleteCreatedBlogs(user) {
  const createdBlogs = await Blog.find({creator: user._id});
  await Promise.all(createdBlogs.map(blog => Blog.findByIdAndDelete(blog._id)));
}

async function removeLikes(user) {
  const likedBlogs = await Blog.find({likes: {$in: user._id}});
  await Promise.all(likedBlogs.map(blog => blog.removeUser(user)));
}

async function deleteUser(user) {
  await User.deleteOne({_id: user._id});
}

then in your request handler do only

await Promise.all([
  deleteCreatedBlogs(user),
  removeLikes(user),
  deleteUser(user),
]);

or alternatively, using .then() instead of the helper functions,

await Promise.all([
  Blog.find({creator: user._id}).then(createdBlogs => 
    Promise.all(createdBlogs.map(blog => Blog.findByIdAndDelete(blog._id)))
  ),
  Blog.find({likes: {$in: user._id}}).then(likedBlogs =>
    Promise.all(likedBlogs.map(blog => blog.removeUser(user)))
  ),
  User.deleteOne({_id: user._id}),
]);

Is this the right use of Promises

1 Answers1