2

I have written a recursive Promise in javascript which seems to be working fine but I wanted to test it using setTimeout() to be sure that I'm awaiting correctly before continuing with the execution. Here is the gist of my code:

try{
  await renameFiles(); // <-- await here
  console.log("do other stuff");
}
catch(){
}

const renameFiles = (path) => {
  return new Promise(resolve => {
    console.log("Renaming files...");

    fs.readdirSync(path).forEach(file) => {
      // if file is a directory ...
      let newPath = path.join(path, file);
      resolve( renameFiles(newPath) ); // <- recursion here!
      // else rename file ...
    }
    resolve();
  })

I've tested it with setTimeout() like this:

const renameFiles = () => {
  return new Promise(resolve => {
    setTimeout(() => {
    // all previous code goes here
    },2000)
  }
}

and the output is:

"Renaming files..."
"Renaming files..."
// bunch of renaming files...
"do other stuff"
"Renaming files..."
"Renaming files..."

So it looks like it's awaiting for a bit but then it continues the execution at some point.

I'm also doubting I'm testing it wrong. Any idea where the problem may be?

Sergio
  • 250
  • 1
  • 4
  • 18
  • 4
    You are calling `resolve` many times, this makes no sense. Per [the spec](https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#Constructor) only the first resolution matters: `If the associated promise has already been resolved, either to a value, a rejection, or another promise, this method does nothing.` – Alexander Azarov May 23 '19 at 19:18
  • 1
    Collect the promises into an array and await Promise.all(arrayOfPromises) – Dominic May 23 '19 at 19:21
  • I think @AlexanderAzarov found the issue. `renameFiles` gets resolved early and the async function continues while the recursion also continues. Possibly just removing that `resolve(renameFiles(newPath))` and putting just `renameFiels(newPath)` will fix it. – Michael Sorensen May 23 '19 at 19:29
  • @MichaelSorensen I've already tried without ```resolve(renameFiles(newPath))``` but it didn't fix it – Sergio May 23 '19 at 19:35

6 Answers6

5

As already mentioned - multiple resolve invocations don't make sense. However that is not the only problem in the code. Root invocation got resolved when its recursive call started for first sub directory. This code will process directories in hierarchical order

rename.js

const fs = require('fs');
const path = require('path');

const inputPath = path.resolve(process.argv[2]);
const newName = 'bar.txt';

async function renameFiles(filePath) {
    for (const file of fs.readdirSync(filePath)) {
        const newPath = path.join(filePath, file);
        const descriptor = fs.lstatSync(newPath);
        if (descriptor.isDirectory()) {
            await renameFiles(newPath)
        } else if (descriptor.isFile()) {
            await renameFile(file);
        }
    }
}

async function renameFile(file) {
    console.log(`Renaming ${file} to ${newName}`)
    return new Promise(resolve => {
       setTimeout(() => {
           console.log(`Renamed ${file} to ${newName}`)
           resolve();
       }, 300)
    });
}

async function main() {
    console.log(`Renaming all files in ${inputPath} to ${newName}`);
    await renameFiles(inputPath);
    console.log('Finished');
}

main();

you can run it like

node rename.js relativeFolderName

or if order doesn't matter, then you can use map and Promise.all as mentioned by @Tiago Coelho

const renameFiles = async path => {
    const renamePromises = fs.readdirSync(path).map(file => {
      if (isDirectory(file)) {
          const newPath = path.join(path, file);
          return renameFiles(newPath)
      } else {
          return renamefile(file);
      }  
    });
    await Promise.all(renamePromises);
}
udalmik
  • 7,838
  • 26
  • 40
  • Good clean solution. Voted up. On your second solution you need to return the promise, not await, and so it does not need to be async. Also you got some extra () after the map that wont really work – Tiago Coelho May 23 '19 at 19:58
  • Right, there is an error in second solution, will fix, thank you. I wanted something like `(async () => console.log('Async!'))()` to get anonymous async/await execution – udalmik May 23 '19 at 20:03
  • It still doesn't' work for me. With the solution given by @udalmik my code looks like this ```return new Promise(resolve=>{ setTimeout(async()=>{ // CODE HERE },2000) ``` which end-up being never resolved. Adding ```resolve()``` after the for loop also end up not awaiting properly. }) – Sergio May 23 '19 at 22:05
  • Sorry, didn't get your problem or what are you trying to solve with setTimeout. Updated answer with test run. – udalmik May 24 '19 at 08:25
  • @udalmik I'm using setTimeout to delay the execution so I can test the "await". If I use setTimeout inside ```async function renameFile(file)``` the program will not await and will continue. – Sergio May 24 '19 at 15:11
  • 1
    What an impressive effort was put in this answer. Kudos for your patience as well. – Alexander Azarov May 24 '19 at 16:34
0

To make this work you need to wait for all the files in the directory to resolve. So you will need to do a Promise.all and use a map instead of a forEach

something like this:

try{
  await renameFiles(); // <-- await here
  console.log("do other stuff");
}
catch(){
}

const renameFiles = (path) => {
  return new Promise(resolve => {
    console.log("Renaming files...");

    const allFilesRenamePromises = fs.readdirSync(path).map(file => {
      if(file.isDirectory()) {
        let newPath = path.join(path, file);
        return renameFiles(newPath); // <- recursion here!
      } else {
        // rename file ...
      }
    }
    resolve(Promise.all(allFilesRenamePromises));
  })
Tiago Coelho
  • 5,023
  • 10
  • 17
  • You should use the original code with `await` from the question though, not the one with `.then()` from Pavithra's answer. – Bergi May 24 '19 at 07:02
0

Instead of writing one big complicated function, I'll suggest a more decomposed approach.

First we start with a files that recursively lists all files at a specified path -

const { readdir, stat } =
  require ("fs") .promises

const { join } =
  require ("path")

const files = async (path = ".") =>
  (await stat (path)) .isDirectory ()
    ? Promise
        .all
          ( (await readdir (path))
              .map (f => files (join (path, f)))
          )
        .then
          ( results =>
             [] .concat (...results)
          )
    : [ path ]

We have a way to list all files now, but we only wish to rename some of them. We'll write a generic search function to find all files that match a query -

const { basename } =
  require ("path")

const search = async (query, path = ".") =>
  (await files (path))
    .filter (x => basename (x) === query)

Now we can write your renameFiles function as a specialisation of search -

const { rename } =
  require ("fs") .promises

const { dirname } =
  require ("path")

const renameFiles = async (from = "", to = "", path = ".") =>
  Promise
    .all
      ( (await search (from, path))
          .map
            ( f =>
                rename
                  ( f
                  , join (dirname (f), to)
                  )
             )
       )

To use it, we simply call renameFiles with its expected parameters -

renameFiles ("foo", "bar", "path/to/someFolder")
  .then
    ( res => console .log ("%d files renamed", res.length)
    , console.error
    )

// 6 files renamed

Reviewing our programs above, we see some patterns emerging with our use of Promise.all, await, and map. Indeed these patterns can be extracted and our programs can be further simplified. Here's files and renameFiles revised to use a generic Parallel module -

const files = async (path = ".") =>
  (await stat (path)) .isDirectory ()
    ? Parallel (readdir (path))
        .flatMap (f => files (join (path, f)))
    : [ path ]

const renameFiles = (from = "", to = "", path = "") =>
  Parallel (search (from, path))
    .map
      ( f =>
          rename
            ( f
            , join (dirname (f), to)
            )
      )

The Parallel module was originally derived in this related Q&A. For additional insight and explanation, please follow the link.

Mulan
  • 129,518
  • 31
  • 228
  • 259
0

In my first answer I showed you how to solve your problem using mainly functional techniques. In this answer, we'll see modern JavaScript features like async iterables make this kind of thing even easier -

const files = async function* (path = ".")
{ if ((await stat (path)) .isDirectory ())
    for (const f of await readdir (path))
      yield* files (join (path, f))
  else
     yield path
}

const search = async function* (query, path = ".")
{ for await (const f of files (path))
    if (query === basename (f))
      yield f
}

const renameFiles = async (from = "", to = "", path = ".") =>
{ for await (const f of search (from, path))
    await rename
      ( f
      , join (dirname (f), to)
      )
}

renameFiles ("foo", "bar", "path/to/someFolder")
  .then (_ => console .log ("done"), console.error)
Mulan
  • 129,518
  • 31
  • 228
  • 259
-1

For completeness, I'll post the entire solution based on @udalmik suggestion. The only difference is that I'm wrapping async function renameFile(file) inside a Promise.

const fs = require('fs');
const path = require('path');

const inputPath = path.resolve(process.argv[2]);
const newName = 'bar.txt';

async function renameFiles(filePath) {
    for (const file of fs.readdirSync(filePath)) {
        const newPath = path.join(filePath, file);
        const descriptor = fs.lstatSync(newPath);
        if (descriptor.isDirectory()) {
            await renameFiles(newPath)
        } else if (descriptor.isFile()) {
            await renameFile(file);
        }
    }
}

async function renameFile(file) {
  return new Promise(resolve => {
    console.log(`Renaming ${file} to ${newName}`);
    resolve();
  })
}

async function main() {
    console.log(`Renaming all files in ${inputPath} to ${newName}`);
    await renameFiles(inputPath);
    console.log('Finished');
}

main();

The reason for using the Promise is that I want to await for all the files to be renamed before continuing the execution (i.e. console.log('Finished');).

I've tested it using setTimeout

return new Promise(resolve => {
    setTimeout(()=>{
      console.log(`Renaming ${file} to ${newName}`);
    },1000)
    resolve(); // edited missing part
  })

The solution took a different path from my original question but I guess it works for me.

Sergio
  • 250
  • 1
  • 4
  • 18
  • Actually you didn't change anything with wrapping the result with fake `Promise` in `readFile`. Please check **Async Functions** section in [this tutorial](https://javascript.info/async-await) – udalmik May 25 '19 at 01:37
  • @udalmik thanks for the link. I think I understand how to use await, but maybe there is something that I miss when using setTimeout?Is there a better/different way to test it? If you don't wrap the function inside a Promise the output will not be "synchronous" ( when using setTimeout() ) – Sergio May 28 '19 at 00:22
  • If you want to emulate some delay of renaming the file - then setTimeout is a way to go, you just missed call to resolve fn, I've added delay to my answer. I wanted to point that no sense to wrap result of async operation with promise (your first code snippet) with immediate resolve call, JS engine will do it for you. – udalmik May 28 '19 at 07:22
  • @udalmik yes, I know there's no need to wrap the result inside a Promise. I did it because when using ```setTimeout``` it was necessary. That's the part that confuses me: why do I need to wrap setTimeout inside a Promise to make it work? – Sergio May 28 '19 at 15:10
-3

try to change the await code like this. this might help you.

try{
  const renameFilesPromise = renameFiles();
  renameFilesPromise.then({      <-- then is a callback when promise is resolved
    console.log("do other stuff");
  })
}
catch(){
}

const renameFiles = (path) => {
  return new Promise(resolve => {
    console.log("Renaming files...");

    fs.readdirSync(path).forEach(file) => {
      // if file is a directory ...
      let newPath = path.join(path, file);
      resolve( renameFiles(newPath) ); // <- recursion here!
      // else rename file ...
    }
    resolve();
  })
  • I haven't tested it yet and it may work, but I'm trying to avoid the callback-hell. I have a bunch of await after that call. – Sergio May 23 '19 at 19:23
  • Ok, you can use the await itself. Try increasing the setTimeOut. Cause setTimeOut automatically resolves in 2000ms. – Pavithra Ravichandran May 23 '19 at 19:30
  • 1
    this will not work either, because the moment you call the first resolve(renameFiles(newPath)) you essentially ignore the remaining files in the loop – Tiago Coelho May 23 '19 at 19:40