Repository: jprichardson/node-fs-extra (js)

Issue:
copy filter called twice
<!-- First ensure you installed the latest version of fs-extra -->
<!-- If your bug still exists please fill out the following information if it applies to your issue: -->
<!-- Please check if you have installed a supported version of Node.js as written in "engines" in the package.json -->
- **Operating System:** MacOS 10.14.6
- **Node.js version:** 14.1.0
- **`fs-extra` version:** 9.0.1

Hi,

Copy filter function called twice for same source and destination. I found a similar report for `emptyDir`, but couldn't see a case for `copy`.

Is this expected for some reason?

Example:

```js
// Below code outputs: 1 2, whereas I expected only 1.
const { copy } = require("fs-extra");

let i = 1;

copy("README.md", "README2.md", {
  filter: () => {
    console.log(i++);
    return true;
  },
});
```






Pull request:
Ensure copy filter fn is not called more than needed
Fixes #809

We need to check filter before creating a parent directory for `dest`, in case the filter cancels the entire operation. After creating the parent (if needed), we were calling a function that rechecked the filter, which wasn't needed, and resulted in the filter fn being called twice at the root level.

With these changes, we should possibly consider renaming some functions, as it doesn't seem to make much sense anymore.

Patch:
diff --git a/docs/copy-sync.md b/docs/copy-sync.md
index 3e5243c..2ccfe3d 100644
--- a/docs/copy-sync.md
+++ b/docs/copy-sync.md
@@ -9,7 +9,7 @@ Copy a file or directory. The directory can have contents.
   - `errorOnExist` `<boolean>`: when `overwrite` is `false` and the destination exists, throw an error. Default is `false`.
   - `dereference` `<boolean>`: dereference symlinks, default is `false`.
   - `preserveTimestamps` `<boolean>`: When true, will set last modification and access times to the ones of the original source files. When false, timestamp behavior is OS-dependent. Default is `false`.
-  - `filter` `<Function>`: Function to filter copied files. Return `true` to include, `false` to exclude.
+  - `filter` `<Function>`: Function to filter copied files/directories. Return `true` to copy the item, `false` to ignore it.
 
 ## Example:
 
diff --git a/docs/copy.md b/docs/copy.md
index 0e66da5..f6e4d98 100644
--- a/docs/copy.md
+++ b/docs/copy.md
@@ -9,7 +9,7 @@ Copy a file or directory. The directory can have contents.
   - `errorOnExist` `<boolean>`: when `overwrite` is `false` and the destination exists, throw an error. Default is `false`.
   - `dereference` `<boolean>`: dereference symlinks, default is `false`.
   - `preserveTimestamps` `<boolean>`: When true, will set last modification and access times to the ones of the original source files. When false, timestamp behavior is OS-dependent. Default is `false`.
-  - `filter` `<Function>`: Function to filter copied files. Return `true` to include, `false` to exclude. Can also return a `Promise` that resolves to `true` or `false` (or pass in an `async` function).
+  - `filter` `<Function>`: Function to filter copied files/directories. Return `true` to copy the item, `false` to ignore it. Can also return a `Promise` that resolves to `true` or `false` (or pass in an `async` function).
 - `callback` `<Function>`
   - `err` `<Error>`
 
diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js
index 2d9523e..5e245b9 100644
--- a/lib/copy-sync/copy-sync.js
+++ b/lib/copy-sync/copy-sync.js
@@ -30,7 +30,7 @@ function handleFilterAndCopy (destStat, src, dest, opts) {
   if (opts.filter && !opts.filter(src, dest)) return
   const destParent = path.dirname(dest)
   if (!fs.existsSync(destParent)) mkdirsSync(destParent)
-  return startCopy(destStat, src, dest, opts)
+  return getStats(destStat, src, dest, opts)
 }
 
 function startCopy (destStat, src, dest, opts) {
diff --git a/lib/copy/copy.js b/lib/copy/copy.js
index c12578d..2a2874f 100644
--- a/lib/copy/copy.js
+++ b/lib/copy/copy.js
@@ -42,10 +42,10 @@ function checkParentDir (destStat, src, dest, opts, cb) {
   const destParent = path.dirname(dest)
   pathExists(destParent, (err, dirExists) => {
     if (err) return cb(err)
-    if (dirExists) return startCopy(destStat, src, dest, opts, cb)
+    if (dirExists) return getStats(destStat, src, dest, opts, cb)
     mkdirs(destParent, err => {
       if (err) return cb(err)
-      return startCopy(destStat, src, dest, opts, cb)
+      return getStats(destStat, src, dest, opts, cb)
     })
   })
 }
diff --git a/lib/empty/index.js b/lib/empty/index.js
index 90fb469..b4a2e82 100644
--- a/lib/empty/index.js
+++ b/lib/empty/index.js
@@ -1,29 +1,20 @@
 'use strict'
 
-const u = require('universalify').fromCallback
-const fs = require('graceful-fs')
+const u = require('universalify').fromPromise
+const fs = require('../fs')
 const path = require('path')
 const mkdir = require('../mkdirs')
 const remove = require('../remove')
 
-const emptyDir = u(function emptyDir (dir, callback) {
-  callback = callback || function () {}
-  fs.readdir(dir, (err, items) => {
-    if (err) return mkdir.mkdirs(dir, callback)
-
-    items = items.map(item => path.join(dir, item))
-
-    deleteItem()
+const emptyDir = u(async function emptyDir (dir) {
+  let items
+  try {
+    items = await fs.readdir(dir)
+  } catch {
+    return mkdir.mkdirs(dir)
+  }
 
-    function deleteItem () {
-      const item = items.pop()
-      if (!item) return callback()
-      remove.remove(item, err => {
-        if (err) return callback(err)
-        deleteItem()
-      })
-    }
-  })
+  return Promise.all(items.map(item => remove.remove(path.join(dir, item))))
 })
 
 function emptyDirSync (dir) {
diff --git a/lib/fs/index.js b/lib/fs/index.js
index 9bbaea4..93fe6a0 100644
--- a/lib/fs/index.js
+++ b/lib/fs/index.js
@@ -61,6 +61,7 @@ Object.keys(fs).forEach(key => {
 api.forEach(method => {
   exports[method] = u(fs[method])
 })
+exports.realpath.native = u(fs.realpath.native)
 
 // We differ from mz/fs in that we still ship the old, broken, fs.exists()
 // since we are a drop-in replacement for the native module
@@ -123,8 +124,3 @@ if (typeof fs.writev === 'function') {
     })
   }
 }
-
-// fs.realpath.native only available in Node v9.2+
-if (typeof fs.realpath.native === 'function') {
-  exports.realpath.native = u(fs.realpath.native)
-}
diff --git a/lib/remove/index.js b/lib/remove/index.js
index cee5340..4428e59 100644
--- a/lib/remove/index.js
+++ b/lib/remove/index.js
@@ -1,9 +1,22 @@
 'use strict'
 
+const fs = require('graceful-fs')
 const u = require('universalify').fromCallback
 const rimraf = require('./rimraf')
 
+function remove (path, callback) {
+  // Node 14.14.0+
+  if (fs.rm) return fs.rm(path, { recursive: true, force: true }, callback)
+  rimraf(path, callback)
+}
+
+function removeSync (path) {
+  // Node 14.14.0+
+  if (fs.rmSync) return fs.rmSync(path, { recursive: true, force: true })
+  rimraf.sync(path)
+}
+
 module.exports = {
-  remove: u(rimraf),
-  removeSync: rimraf.sync
+  remove: u(remove),
+  removeSync
 }


Test patch:
diff --git a/lib/copy-sync/__tests__/copy-sync-file.test.js b/lib/copy-sync/__tests__/copy-sync-file.test.js
index 6ffff60..f656e04 100644
--- a/lib/copy-sync/__tests__/copy-sync-file.test.js
+++ b/lib/copy-sync/__tests__/copy-sync-file.test.js
@@ -104,6 +104,25 @@ describe('+ copySync() / file', () => {
       assert(fs.existsSync(destFile3))
     })
 
+    it('should not call filter fn more than needed', () => {
+      const src = path.join(TEST_DIR, 'foo')
+
+      fs.writeFileSync(src, '')
+
+      const dest = path.join(TEST_DIR, 'bar')
+
+      let filterCallCount = 0
+      const filter = () => {
+        filterCallCount++
+        return true
+      }
+
+      fs.copySync(src, dest, filter)
+
+      assert.strictEqual(filterCallCount, 1)
+      assert(fs.existsSync(dest))
+    })
+
     describe('> when the destination dir does not exist', () => {
       it('should create the destination directory and copy the file', () => {
         const src = path.join(TEST_DIR, 'file.txt')
diff --git a/lib/copy/__tests__/copy.test.js b/lib/copy/__tests__/copy.test.js
index e7eed97..71470a2 100644
--- a/lib/copy/__tests__/copy.test.js
+++ b/lib/copy/__tests__/copy.test.js
@@ -283,6 +283,25 @@ describe('fs-extra', () => {
         })
       })
 
+      it('should not call filter fn more than needed', done => {
+        const src = path.join(TEST_DIR, 'foo')
+        fs.writeFileSync(src, '')
+        const dest = path.join(TEST_DIR, 'bar')
+
+        let filterCallCount = 0
+        const filter = () => {
+          filterCallCount++
+          return true
+        }
+
+        fse.copy(src, dest, filter, err => {
+          assert(!err)
+          assert.strictEqual(filterCallCount, 1)
+          assert(fs.existsSync(dest))
+          done()
+        })
+      })
+
       it('accepts options object in place of filter', done => {
         const srcFile1 = path.join(TEST_DIR, '1.jade')
         fs.writeFileSync(srcFile1, '')