Asynchronous array methods chaining

Question

I am trying to check that a list of files are all images using functional programming.

Some actions I go through are asynchronous, so I have to wait for them to completely finish before passing to a new one.

const filteredFiles = ['favicon.ico', 'site.webmanifest', 'browserconfig.xml'];
const fileBuffers = await Promise.all(
    directory.files
        .filter(file => !filteredFiles.includes(file.path))
        // get a buffer containing the image file
        .map(async file => await file.buffer())
)
const everyFileIsImage = (await Promise.all(
    // check that this buffer has correct mime type
    fileBuffers.map(async buffer => await this.bufferIsImageFile(buffer))
)).every(isImage => isImage);

if (!everyFileIsImage) {
    throw new BadRequestException(`All files except ${filteredFiles} must be images`);
}

Here is the bufferIsImageFile method which is using the file-type library

private async bufferIsImageFile(buffer) {
    const type = await FileType.fromBuffer(buffer);

    return type !== undefined && type.mime.startsWith('image/');
}

I am not all satisfied by the piece of code. But I'm also not sure on how it could be improved.

What I'd like the most would be to avoid using those Promise.all, but is it even possible?

Maybe should I use a for loop instead?

Answer 1

Some actions I go through are asynchronous, so I have to wait for them to completely finish before passing to a new one.

That's right. However, your interpretation of that statement is wrong.

The actions need to be serialized, but only on a per file basis.

const buffer = await file.buffer();
return await this.bufferIsImageFile(buffer);

Concurrency

You can further proceed with Promise.all as you did, but it has a few caveats.

• Since any file that is not image means you should return false, not caring about the others. And so you should be able to cancel all the other file scans once you find the first file that is not image. Native promises are not cancelable but I am sure there are user land implementations.
• You have no control about the number of files consumers will ask for. If they ask for a lot of files, you start a lot of async tasks and this may consume a lot of memory. You may want to only start a limited amount of tasks at a time and add more once some finishes.

The latter can be solved using Promise.race, but the actual implementation is not that trivial and so I will not write this for you :)

Maybe this SO question might be of help here: https://stackoverflow.com/questions/42896456/get-which-promise-completed-in-promise-race

Or you may want to use some promises wrapper like bluebird which also provides cancellation mechanism (http://bluebirdjs.com)

Filtering

The filteredFiles list seems to me too hard coded (but it all seems like you pulled it together just for this review, in which case you may consider this irrelevant).

It should be injected to the method somehow, your options include:

• method argument (thats awkward tho)
• class property injected through constructor
• decorator pattern (https://lmgtfy.com/?q=decorator+pattern+nodejs)

Answer 2

• Consider using Set for filteredFiles. I know it is a small array to iterates through for calls to Array.includes(), but it is just general good practice IMO to use an appropriate data structure for a key lookup type use case.
• asynchronous functions are not supported inside map() though in this case it doesn’t really matter as you are just mapping an array of promises. But in the future just know that if your are deepening on await in such a function to work as expected, it won’t.