Refactor code with promises to read files and convert them to json

Question

I'm trying to do the following: Read the content of a directory to find all the .xml files (I'm using glob but I'd like to use something like fs.readdir from fs), then I want to read every file using fs.readFile and then I want to convert the xml file to JSON objects. I'm using xml2json for this purpose.

Once I have the json objects, I would like to iterate every one of them to get the one property out of it and push it to an array. Eventually, all the code is wrapped in a function that logs the content of the array (once is completed). This code currently works fine but I'm getting to the famous callback hell.

const fs = require('fs');
const glob = require('glob');
const parser = require('xml2json');
let connectors = [] 


function getNames(){
glob(__dirname + '/configs/*.xml', {}, (err, files) => {
     for (let j=0; j < files.length; j++) {
          fs.readFile( files[j], function(err, data) {
             try {
                   let json = parser.toJson(data, {object: true, alternateTextNode:true, sanitize:true})               
                   for (let i=0; i< json.properties.length; i++){
                     connectors.push(json.properties[i].name)
                   if (connectors.length === files.length){return console.log(connectors)}
                   }
              }
              catch(e){
                   console.log(e)
              }
       });
     }
 })
}
getNames()

However, I'd like to move to a more clean and elegant solution (using promises). I've been reading the community and I've found some ideas in some similar posts here or here.

I'd like to have your opinion on how I should proceed for this kind of situations. Should I go for a sync version of readFile instead? Should I use promisifyAll to refactor my code and use promises everywhere? If so, could you please elaborate on what my code should look like?

I've also learned that there's a promises based version of fs from node v10.0.0 onwards. Should I go for that option? If so how should I proceed with the parser.toJson() part. I've also seen that there's another promise-based version called xml-to-json-promise.

I'd really appreciate your insights into this as I'm not very familiar with promises when there are several asynchronous operations and loops involved, so I end up having dirty solutions for situations like this one.

Regards, J

What is the logic behind `connectors.length === files.length`? What does the one have to do with the other? — trincot, Feb 22 '20 at 21:16
@trincot It's a way to know when the connectors array has been fully populated. When the length of the array to fill (connectors) is equal to the number of files. Then I return the console log with the array filled when the function is called. — Javilondo Ramires, Feb 22 '20 at 21:20
I don't get it: per file you can have several properties, so how is the number of files relevant for the total number of properties? Also, be aware that that `return` will not interrupt the processing, there can still be more `readFile` callbacks that will execute. — trincot, Feb 22 '20 at 21:24
The idea is that files is an array composed of x file names that are intended to be read by fs.ReadFile. That if statement was only meant to check that the connectors array is fully populated (it has the same number of elements as the number of files). But now that I think again, you're right as there can be several properties per file. How can I know when the array has been filled? Thanks for pointing this out — Javilondo Ramires, Feb 22 '20 at 21:31

trincot · Accepted Answer · 2020-02-25T16:45:14.043

I would indeed suggest that you use the promise-version of glob and fs, and then use async, await and Promise.all to get it all done.

NB: I don't see the logic about the connectors.length === files.length check, as in theory the number of connectors (properties) can be greater than the number of files. I assume you want to collect all of them, irrespective of their number.

So here is how the code could look (untested):

const fs = require('fs').promises; // Promise-version (node 10+)
const glob = require('glob-promise'); // Promise-version
const parser = require('xml2json');

async function getNames() {
    let files = await glob(__dirname + '/configs/*.xml');
    let promises = files.map(fileName => fs.readFile(fileName).then(data =>
        parser.toJson(data, {object: true, alternateTextNode:true, sanitize:true})
              .properties.map(prop => prop.name)
    ));
    return (await Promise.all(promises)).flat();
}

getNames().then(connectors => {
    // rest of your processing that needs access to connectors...
});

As in comments you write that you have problems with accessing properties.map, perform some validation, and skip cases where there is no properties:

const fs = require('fs').promises; // Promise-version (node 10+)
const glob = require('glob-promise'); // Promise-version
const parser = require('xml2json');

async function getNames() {
    let files = await glob(__dirname + '/configs/*.xml');
    let promises = files.map(fileName => fs.readFile(fileName).then(data =>
        (parser.toJson(data, {object: true, alternateTextNode:true, sanitize:true})
              .properties || []).map(prop => prop.name)
    ));
    return (await Promise.all(promises)).flat();
}

getNames().then(connectors => {
    // rest of your processing that needs access to connectors...
});

It makes a 1D array of a 2D one (chunks of connectors). If you don't have support for it, it can also be done with `[].concat(...await Promise.all(promises))`. — trincot, Feb 22 '20 at 22:09
@trincot the idea is great and it seems to be the way forward. I'm however having an error that says (node:41475) UnhandledPromiseRejectionWarning: TypeError: parser.toJson(...).properties.map is not a function. However, if I replace the prop=> prop.name with prop=> console.log(prop.name) it logs all the values I want. What can be the issue? — Javilondo Ramires, Feb 25 '20 at 16:39
That means that you have cases where `toJson()` returns an object that does not have a `properties` property. So you should probably first save the result of `toJson()` in a variable, then check if it has that `properties`, and then decide what you want to happen in either case... — trincot, Feb 25 '20 at 16:42
See addition to answer on how you can deal with missing `properties`. — trincot, Feb 25 '20 at 16:45

Refactor code with promises to read files and convert them to json

1 Answers1