fix: move to async functions where possible (#106)

This refactors the code to use async functions where possible.
It also uses `@npmcli/fs` consistently (it was already a dependency,
just not used).
It also reorders the test files to match their sources in `./lib`.

Tests were not refactored (except where needed to move to `@npmcli/fs`)
in the interest of an easier PR review.

.eslintrc.local.js

@@ -1,5 +1,8 @@
+'use strict'
+
 module.exports = {
   rules: {
+    strict: 'error',
     'no-shadow': 0, // XXX: fix this later
   },
 }

lib/content/read.js

@@ -1,42 +1,35 @@
 'use strict'
 
-const util = require('util')
-
-const fs = require('fs')
+const fs = require('@npmcli/fs')
 const fsm = require('fs-minipass')
 const ssri = require('ssri')
 const contentPath = require('./path')
 const Pipeline = require('minipass-pipeline')
 
-const lstat = util.promisify(fs.lstat)
-const readFile = util.promisify(fs.readFile)
-const copyFile = util.promisify(fs.copyFile)
-
 module.exports = read
 
 const MAX_SINGLE_READ_SIZE = 64 * 1024 * 1024
-function read (cache, integrity, opts = {}) {
+async function read (cache, integrity, opts = {}) {
   const { size } = opts
-  return withContentSri(cache, integrity, (cpath, sri) => {
+  const { stat, cpath, sri } = await withContentSri(cache, integrity, async (cpath, sri) => {
     // get size
-    return lstat(cpath).then(stat => ({ stat, cpath, sri }))
-  }).then(({ stat, cpath, sri }) => {
-    if (typeof size === 'number' && stat.size !== size) {
-      throw sizeError(size, stat.size)
-    }
+    const stat = await fs.lstat(cpath)
+    return { stat, cpath, sri }
+  })
+  if (typeof size === 'number' && stat.size !== size) {
+    throw sizeError(size, stat.size)
+  }
 
-    if (stat.size > MAX_SINGLE_READ_SIZE) {
-      return readPipeline(cpath, stat.size, sri, new Pipeline()).concat()
-    }
+  if (stat.size > MAX_SINGLE_READ_SIZE) {
+    return readPipeline(cpath, stat.size, sri, new Pipeline()).concat()
+  }
 
-    return readFile(cpath, null).then((data) => {
-      if (!ssri.checkData(data, sri)) {
-        throw integrityError(sri, cpath)
-      }
+  const data = await fs.readFile(cpath, { encoding: null })
+  if (!ssri.checkData(data, sri)) {
+    throw integrityError(sri, cpath)
+  }
 
-      return data
-    })
-  })
+  return data
 }
 
 const readPipeline = (cpath, size, sri, stream) => {
@@ -58,7 +51,7 @@ module.exports.sync = readSync
 function readSync (cache, integrity, opts = {}) {
   const { size } = opts
   return withContentSriSync(cache, integrity, (cpath, sri) => {
-    const data = fs.readFileSync(cpath)
+    const data = fs.readFileSync(cpath, { encoding: null })
     if (typeof size === 'number' && size !== data.length) {
       throw sizeError(size, data.length)
     }
@@ -77,16 +70,19 @@ module.exports.readStream = readStream
 function readStream (cache, integrity, opts = {}) {
   const { size } = opts
   const stream = new Pipeline()
-  withContentSri(cache, integrity, (cpath, sri) => {
-    // just lstat to ensure it exists
-    return lstat(cpath).then((stat) => ({ stat, cpath, sri }))
-  }).then(({ stat, cpath, sri }) => {
+  // Set all this up to run on the stream and then just return the stream
+  Promise.resolve().then(async () => {
+    const { stat, cpath, sri } = await withContentSri(cache, integrity, async (cpath, sri) => {
+      // just lstat to ensure it exists
+      const stat = await fs.lstat(cpath)
+      return { stat, cpath, sri }
+    })
     if (typeof size === 'number' && size !== stat.size) {
       return stream.emit('error', sizeError(size, stat.size))
     }
 
     readPipeline(cpath, stat.size, sri, stream)
-  }, er => stream.emit('error', er))
+  }).catch(err => stream.emit('error', err))
 
   return stream
 }
@@ -96,7 +92,7 @@ module.exports.copy.sync = copySync
 
 function copy (cache, integrity, dest) {
   return withContentSri(cache, integrity, (cpath, sri) => {
-    return copyFile(cpath, dest)
+    return fs.copyFile(cpath, dest)
   })
 }
 
@@ -108,14 +104,17 @@ function copySync (cache, integrity, dest) {
 
 module.exports.hasContent = hasContent
 
-function hasContent (cache, integrity) {
+async function hasContent (cache, integrity) {
   if (!integrity) {
-    return Promise.resolve(false)
+    return false
   }
 
-  return withContentSri(cache, integrity, (cpath, sri) => {
-    return lstat(cpath).then((stat) => ({ size: stat.size, sri, stat }))
-  }).catch((err) => {
+  try {
+    return await withContentSri(cache, integrity, async (cpath, sri) => {
+      const stat = await fs.lstat(cpath)
+      return { size: stat.size, sri, stat }
+    })
+  } catch (err) {
     if (err.code === 'ENOENT') {
       return false
     }
@@ -128,7 +127,7 @@ function hasContent (cache, integrity) {
         return false
       }
     }
-  })
+  }
 }
 
 module.exports.hasContent.sync = hasContentSync
@@ -159,61 +158,47 @@ function hasContentSync (cache, integrity) {
   })
 }
 
-function withContentSri (cache, integrity, fn) {
-  const tryFn = () => {
-    const sri = ssri.parse(integrity)
-    // If `integrity` has multiple entries, pick the first digest
-    // with available local data.
-    const algo = sri.pickAlgorithm()
-    const digests = sri[algo]
-
-    if (digests.length <= 1) {
-      const cpath = contentPath(cache, digests[0])
-      return fn(cpath, digests[0])
-    } else {
-      // Can't use race here because a generic error can happen before
-      // a ENOENT error, and can happen before a valid result
-      return Promise
-        .all(digests.map((meta) => {
-          return withContentSri(cache, meta, fn)
-            .catch((err) => {
-              if (err.code === 'ENOENT') {
-                return Object.assign(
-                  new Error('No matching content found for ' + sri.toString()),
-                  { code: 'ENOENT' }
-                )
-              }
-              return err
-            })
-        }))
-        .then((results) => {
-          // Return the first non error if it is found
-          const result = results.find((r) => !(r instanceof Error))
-          if (result) {
-            return result
-          }
-
-          // Throw the No matching content found error
-          const enoentError = results.find((r) => r.code === 'ENOENT')
-          if (enoentError) {
-            throw enoentError
-          }
-
-          // Throw generic error
-          throw results.find((r) => r instanceof Error)
-        })
+async function withContentSri (cache, integrity, fn) {
+  const sri = ssri.parse(integrity)
+  // If `integrity` has multiple entries, pick the first digest
+  // with available local data.
+  const algo = sri.pickAlgorithm()
+  const digests = sri[algo]
+
+  if (digests.length <= 1) {
+    const cpath = contentPath(cache, digests[0])
+    return fn(cpath, digests[0])
+  } else {
+    // Can't use race here because a generic error can happen before
+    // a ENOENT error, and can happen before a valid result
+    const results = await Promise.all(digests.map(async (meta) => {
+      try {
+        return await withContentSri(cache, meta, fn)
+      } catch (err) {
+        if (err.code === 'ENOENT') {
+          return Object.assign(
+            new Error('No matching content found for ' + sri.toString()),
+            { code: 'ENOENT' }
+          )
+        }
+        return err
+      }
+    }))
+    // Return the first non error if it is found
+    const result = results.find((r) => !(r instanceof Error))
+    if (result) {
+      return result
     }
-  }
 
-  return new Promise((resolve, reject) => {
-    try {
-      tryFn()
-        .then(resolve)
-        .catch(reject)
-    } catch (err) {
-      reject(err)
+    // Throw the No matching content found error
+    const enoentError = results.find((r) => r.code === 'ENOENT')
+    if (enoentError) {
+      throw enoentError
     }
-  })
+
+    // Throw generic error
+    throw results.find((r) => r instanceof Error)
+  }
 }
 
 function withContentSriSync (cache, integrity, fn) {

lib/content/rm.js

@@ -8,13 +8,13 @@ const rimraf = util.promisify(require('rimraf'))
 
 module.exports = rm
 
-function rm (cache, integrity) {
-  return hasContent(cache, integrity).then((content) => {
-    // ~pretty~ sure we can't end up with a content lacking sri, but be safe
-    if (content && content.sri) {
-      return rimraf(contentPath(cache, content.sri)).then(() => true)
-    } else {
-      return false
-    }
-  })
+async function rm (cache, integrity) {
+  const content = await hasContent(cache, integrity)
+  // ~pretty~ sure we can't end up with a content lacking sri, but be safe
+  if (content && content.sri) {
+    await rimraf(contentPath(cache, content.sri))
+    return true
+  } else {
+    return false
+  }
 }

lib/content/write.js

@@ -4,7 +4,7 @@ const util = require('util')
 
 const contentPath = require('./path')
 const fixOwner = require('../util/fix-owner')
-const fs = require('fs')
+const fs = require('@npmcli/fs')
 const moveFile = require('../util/move-file')
 const Minipass = require('minipass')
 const Pipeline = require('minipass-pipeline')
@@ -15,8 +15,6 @@ const ssri = require('ssri')
 const uniqueFilename = require('unique-filename')
 const fsm = require('fs-minipass')
 
-const writeFile = util.promisify(fs.writeFile)
-
 module.exports = write
 
 async function write (cache, data, opts = {}) {
@@ -36,7 +34,7 @@ async function write (cache, data, opts = {}) {
 
   const tmp = await makeTmp(cache, opts)
   try {
-    await writeFile(tmp.target, data, { flag: 'wx' })
+    await fs.writeFile(tmp.target, data, { flag: 'wx' })
     await moveToDestination(tmp, cache, sri, opts)
     return { integrity: sri, size: data.length }
   } finally {
@@ -115,7 +113,7 @@ async function handleContent (inputStream, cache, opts) {
   }
 }
 
-function pipeToTmp (inputStream, cache, tmpTarget, opts) {
+async function pipeToTmp (inputStream, cache, tmpTarget, opts) {
   let integrity
   let size
   const hashStream = ssri.integrityStream({
@@ -143,30 +141,27 @@ function pipeToTmp (inputStream, cache, tmpTarget, opts) {
     outStream
   )
 
-  return pipeline.promise().then(() => ({ integrity, size }))
+  await pipeline.promise()
+  return { integrity, size }
 }
 
-function makeTmp (cache, opts) {
+async function makeTmp (cache, opts) {
   const tmpTarget = uniqueFilename(path.join(cache, 'tmp'), opts.tmpPrefix)
-  return fixOwner.mkdirfix(cache, path.dirname(tmpTarget)).then(() => ({
+  await fixOwner.mkdirfix(cache, path.dirname(tmpTarget))
+  return {
     target: tmpTarget,
     moved: false,
-  }))
+  }
 }
 
-function moveToDestination (tmp, cache, sri, opts) {
+async function moveToDestination (tmp, cache, sri, opts) {
   const destination = contentPath(cache, sri)
   const destDir = path.dirname(destination)
 
-  return fixOwner
-    .mkdirfix(cache, destDir)
-    .then(() => {
-      return moveFile(tmp.target, destination)
-    })
-    .then(() => {
-      tmp.moved = true
-      return fixOwner.chownr(cache, destination)
-    })
+  await fixOwner.mkdirfix(cache, destDir)
+  await moveFile(tmp.target, destination)
+  tmp.moved = true
+  await fixOwner.chownr(cache, destination)
 }
 
 function sizeError (expected, found) {