.then() does not appear to be waiting for the previous .then()

Question

I'm creating a process that converts multiple markdown files into a single pdf. It creates a pdf file for each .md file found in the source directory. Then it merges the individual pdf files into one pdf. It is this last step that is failing saying the individual pdf files do not exist.

const markdownpdf = require('markdown-pdf')
const path = require('path')
const PDFMerge = require('pdf-merge')
const fse = require('fs-extra')

const srcDir = '../manuscript'
const outDir = 'out'

const main = () => {

    fse.pathExists(outDir)
    .then(() => {
      fse.remove(outDir).then(() => {
        fse.ensureDir(outDir)
      }).then(() => {
        return fse.readdir(srcDir)
      }).then((srcDirFiles) => {
        console.log('source directory file count = ', srcDirFiles.length)
        return srcDirFiles.filter(f => path.extname(f) === '.md')
      }).then((mdFiles) => {
        console.log('number of md files', mdFiles.length);
        return mdFiles.map(file => {

          const outFileName = `${path.basename(file, '.md')}.pdf`
          fse.createReadStream(`${srcDir}/${file}`)
            .pipe(markdownpdf())
            .pipe(fse.createWriteStream(`${outDir}/${outFileName}`))
          return `${outDir}/${outFileName}`
        })
      }).then(outFiles => {
        console.log('number of pdf files created =', outFiles.length)
        PDFMerge(outFiles, { output: `${__dirname}/3.pdf`  })
      })
    })
}

main()

If I wrap the PDFMerge() line in setTimeout() it does work

setTimeout(() => {
  PDFMerge(outFiles, { output: `${__dirname}/3.pdf` })
}, 1000)

I'm wondering why the setTimeout() is needed and what needs to be changed so it isn't.

I also wrote an async/await version that had the same problem and also worked with setTimeOut()

Edit

In response to Zach Holt's suggestion, here is the async/await version:

const markdownpdf = require('markdown-pdf')
const path = require('path')
const PDFMerge = require('pdf-merge')
const fse = require('fs-extra')

const srcDir = '../manuscript'
const outDir = 'out'

const createPdf = async (file) => {
  try {
    const outFileName = `${path.basename(file, '.md')}.pdf`
    await fse.createReadStream(`${srcDir}/${file}`)
      .pipe(markdownpdf())
      .pipe(await fse.createWriteStream(`${outDir}/${outFileName}`))
  }
  catch (e) {
    console.log(e)
  }
}

const makePdfFiles = (files) => {
  files.forEach(file => {
    if (path.extname(file) === '.md') {
      createPdf(file)
    }
  })
}

const mergeFiles = async (files) => {
  try {
    await PDFMerge(files, {output: `${__dirname}/3.pdf`})
  }
  catch (e) {
    console.log(e)

  }
}

const addPathToPdfFiles = (files) => {
  return files.map(file => {
    return `${outDir}/${file}`
  })
}

const main = async () => {
  try {
    const exists = await fse.pathExists(outDir)
    if (exists) {
      await fse.remove(outDir)
    }
    await fse.ensureDir(outDir)
    const mdFiles = await fse.readdir(srcDir)
    const filesMade = await makePdfFiles(mdFiles)
    const pdfFiles = await fse.readdir(outDir)
    const pdfFilesWithPath = addPathToPdfFiles(pdfFiles)
    mergeFiles(pdfFilesWithPath)
    // setTimeout(() => {
    //   mergeFiles(pdfFilesWithPath)
    // }, 1000)
  } catch (e) {
    console.log(e)
  }
}

It has the same problem.

I also tried:

const makePdfFiles = files => {
  return new Promise((resolve, reject) => {
    try {
      files.forEach(file => {
        if (path.extname(file) === '.md') {
          createPdf(file)
        }
      })
      resolve(true)
    } catch (e) {
      reject(false)
      console.log('makePdfFiles ERROR', e)
    }
  })
}

But it made no difference.

I would highly recommend using async-await, to avoid this "then hell" — i.brod, May 22 '19 at 01:02

score 2 · Answer 1 · answered May 21 '19 at 20:56

2

You need to return the promise from ensureDir() to make it wait for it.

answered May 21 '19 at 20:56

SLaks

868,454
176
1,908
1,964

And all the other promises too – baao May 21 '19 at 21:21
I think if that were the case, the code would fail earlier when writing the individual pdf files. – klequis May 21 '19 at 21:24
@klequis your code is NOT waiting for `ensureDir` but that does not seem to break it even when not waiting... since you don't return the Promises on multiple places, one of the other places breaks it. – Aprillion May 21 '19 at 21:39
bambam and Aprillion: I see now you are both right about not returning the promise. I thought the return was only needed if the promise returned a value to be used in the next promise. I see now that isn't the case. However, after correcting that It still didn't work and I think the problem was not being able to wait for the .pipe()s. Found a different solution which I'll put below for reference. – klequis May 22 '19 at 17:00

score 1 · Answer 2 · answered May 21 '19 at 21:01

1

I think the issue might be that you're creating a read stream for each of the .md files, but not waiting for the reads to finish before trying to merge outFiles.

You could likely wait until the outFiles length is the same as the number of md files found before merging.

Also, you should stick with async/await for this. It'll keep the code much clearer

answered May 21 '19 at 21:01

Zak Holt

347
3
6

That was my thought as well. You can see my failed attempt to fix that at the bottom of the async/await code I just added to the question. – klequis May 21 '19 at 21:22

score 1 · Answer 3 · answered May 21 '19 at 21:32

Let me over-simplify your code to illustrate the problem:

p1.then(() => {
  p2.then().then().then()
}).then(/* ??? */)

which is the same as:

p1.then(() => {
  p2.then().then().then()
  return undefined
}).then(/* undefined */)

What you need for chaining is to return the inner Promise:

p1.then(() => // no {code block} here, just return value
  p2.then().then().then()
).then(/* ??? */)

which is the same as:

p1.then(() => {
  p3 = p2.then()
  p4 = p3.then()
  p5 = p4.then()
  return p5
}).then(/* p5 */)

klequis · Answer 4 · 2019-05-22T21:06:57.117

As far as I can tell the original problem was the approach and not the obvious errors correctly pointed out by others. I found a much simpler solution to the overall goal of producing a single pdf from multiple md files.

const markdownpdf = require('markdown-pdf')
const path = require('path')
const fse = require('fs-extra')

const srcDir = '../manuscript'

const filterAndAddPath = (files) => {
  try {
    const mdFiles = files
      .filter(f => path.extname(f) === '.md')
      .map(f => `${srcDir}/${f}`)
    return mdFiles
  }
  catch (e) {
    console.log('filterAndAddPath', e)

  }
}

const main4 = async ()  => {
  const allFiles = await fse.readdir(srcDir)
  const mdFiles = filterAndAddPath(allFiles)
  const bookPath = 'book.pdf'
  markdownpdf()
    .concat.from(mdFiles)
    .to(bookPath, function() {
      console.log('Created', bookPath)
    })
}

main4()

`filterAndAddPath` is not async, so there is no point in using a promise. — SLaks, May 22 '19 at 18:05

.then() does not appear to be waiting for the previous .then()

4 Answers4