1

I am discovering all kinds of gaps in my understanding during this project. I am working on building an intermediate API server. Some end-points take an array of info, then make an external call for each piece of info, then do some manipulation of the response, and ultimately needs to send a response when everything is done.

I am usually handling a single call at a time, and thought I had a decent grasp of Async Await. I used Promise.all() like three years ago for some parallel calls, but it's been an age, and this doesn't quite translate the same way. I also tried making each step it's on thing and chaining .then()s however some of my functions didn't seen async/ wouldn't trigger a .then(). All the documentation I have looked at has gotten me partway there, but then I am still getting quite stuck.

const createUserDir = async (req, res) =>{
    // make User Directory
    fs.mkdir(path.join(__dirname, `../Users/${req.body.userID}`), { recursive: true },(err)=>{
        if (err) {
            return console.error(err);
        }
        getAllAssetImages(req.body.assets, req.body.config, req.body.userID, res)
    })
}

const getAllAssetImages =async (assets, config, userID, response)=> {
    
    try {
        const allAssets = await Promise.all(
            assets.map(asset => (
// utils.getAsset is an axios call to get a file, this works
                utils.getAsset(asset, config)
                .then(res => fs.mkdir(path.join(__dirname, `../Users/${userID}/${asset.assetId}`),
                ()=>fs.writeFile(path.join(__dirname, `../Users/${userID}/${asset.assetId}/${asset.assetId}.pptx`), res.data, {encoding: 'binary'}, async (err) => {
                    if (err){
                        console.log(err.code)
                    } else {
                        console.log("File written successfully")
//convertPPTS.convertPPTS is an async function that uses ppt-png npm package: works    
                    convertPPTS.convertPPTS([path.join(__dirname,`../Users/${userID}/${asset.assetId}/${asset.assetId}.pptx`)],path.join(__dirname,`../Users/${userID}/${asset.assetId}/`))
                       
                    } 
                }
            ))
        )
            )))
            .then(()=>{
                console.log('please log at the end')
                response.send({status:200, body:'assets available!'})
    })
            console.log('all the promises', allAssets)
        } catch (err){
            throw Error(err)
        }
}

router.post('/', (req,res) => {
     createUserDir(req, res)
     .then(()=>{console.log('all assets written')})
})


export default { router }

It will often finish fetching, writing converting 2 out of three files then say it is done, before it is, then throw an error when it tries to convert a file that isn't there yet.

halfer
  • 19,824
  • 17
  • 99
  • 186
Jeffrey Bentley
  • 43
  • 5
  • Start by getting rid of the `fs.mkdir` and `fs.writeFile` callbacks. Use `fs/promises` instead of `fs`. – Bergi Apr 12 '23 at 02:55
  • fs/promises had some notation around performance, so I avoided them as I understood it a little less. That being said, I am sure I am making all kinds of performance sacrifices – Jeffrey Bentley Apr 12 '23 at 21:10
  • Not sure what you mean by "*some notation around performance*". You should definitely use the module (unless you need to support old node.js versions that don't provide it), focus on clean and readable code first to get it working properly. You will not notice a performance difference - and even if you do, you should not prematurely optimise but only after you have a working version. – Bergi Apr 13 '23 at 00:40

1 Answers1

0

The biggest thing I did, was remove the mixing of .then() and awaiting promises, leaning into promises and async await. I believe there is room to optimize by allowing some of this to run in parallel, and changing what is being awaited.

const createUserDir = async (req) => {
  let { assets, userID, SP } = req.body;

  fs.mkdir(path.join(__dirname, `../Users/${req.body.userID}`), { recursive: true }, (err) => {
      if (err) {
        console.error(err.message)
        return (err);
      }
    }
  );
  await getAllAssetImages(assets, SP, userID);
};

const getAllAssetImages = async (assets, config, userID) => {
  try {
    await Promise.all(
      assets.map(async (asset) => {
        let res = await showpadUtils.getAsset(asset, config);
        await new Promise((resolve, reject) =>
          fs.mkdir( path.join(__dirname, `../Users/${userID}/${asset.assetId}`), () => (
            fs.writeFile(path.join(__dirname, `../Users/${userID}/${asset.assetId}/${asset.assetId}.pptx`),
                res.data,
                { encoding: "binary" },
                async (err) => {
                  if (err) {
                    console.log(err.code);
                  } else {
                    console.log("File written successfully");
                    resolve(
                      convertPPTS.convertPPTS(
                        [
                          path.join(__dirname, `../Users/${userID}/${asset.assetId}/${asset.assetId}.pptx`),
                        ],
                        path.join(__dirname, `../Users/${userID}/${asset.assetId}/`)
                      )
                    );
                  }
                }
              )
          )
          )
        );
      })
    );

    console.log("please log at the end");
} catch (err) {
    throw Error(err);
}
};

router.post("/", async (req, res) => {
    await createUserDir(req);
    console.log("all assets written");
    res.send({ status: 200, body: "assets available!" });
});
Jeffrey Bentley
  • 43
  • 5
  • Don't use an `async` function as the callback to `fs.writeFile` - you're not `await`ing anything, and even if you did, you should rather promisify only `writeFile` and not wrap the business logic. Also, instead of `if (err) { console.log(err.code); }` do call `reject`, and the `} catch (err) { throw Error(err); }` is pointless and should be omitted. – Bergi Apr 13 '23 at 00:42
  • You're not waiting for `fs.mkdir(path.join(__dirname, `../Users/${req.body.userID}`), …)` to finish. – Bergi Apr 13 '23 at 00:43