Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter option doesn't work the same way in copy and copySync #180

Closed
stefk opened this issue Sep 25, 2015 · 7 comments
Closed

Filter option doesn't work the same way in copy and copySync #180

stefk opened this issue Sep 25, 2015 · 7 comments
Assignees
Milestone

Comments

@stefk
Copy link

stefk commented Sep 25, 2015

copySync applies the filter only to files, while copy applies the filter to every entry.

So if you have the following directory structure:

src
  foo
    baz
      file1 
  bar
    baz
      file2

calling copy with { filter: /baz/ } will produce:

dest
  foo
  bar

wheras copySync will do:

dest
  foo
    baz
  bar
    baz

The former is obviously what's expected.

@jprichardson
Copy link
Owner

Thanks for pointing this out :)

@jehendricks
Copy link

I've been experiencing the same problem and I think I may have found what is going on. In copy-sync, the check for filter that sets performCopy is only checked inside the stats.isFile() conditional. I moved the setting of performCopy outside the conditional then wrapped the whole conditional (isFile, isDirectory, isSymbolicLink) in a performCopy conditional. That worked, but of course I may have broken some other internal code functionality in the process. Here's my code fragment if it's any help:

ar performCopy = false

if (options.filter instanceof RegExp) performCopy = options.filter.test(src)
else if (typeof options.filter === 'function') performCopy = options.filter(src)

if (performCopy) {
if (stats.isFile()) {
if (!destFolderExists) mkdir.mkdirsSync(destFolder)
copyFileSync(src, dest, {clobber: options.clobber, preserveTimestamps: options.preserveTimestamps})
} else if (stats.isDirectory()) {
if (!fs.existsSync(dest)) mkdir.mkdirsSync(dest)
var contents = fs.readdirSync(src)
contents.forEach(function (content) {
var opts = options
opts.recursive = true
copySync(path.join(src, content), path.join(dest, content), opts)
})
} else if (options.recursive && stats.isSymbolicLink()) {
var srcPath = fs.readlinkSync(src)
fs.symlinkSync(srcPath, dest)
}
}

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 26, 2016

@jprichardson Should this added to the v1.0.0 milestone?

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 28, 2016

@jprichardson ping?

@jprichardson
Copy link
Owner

@jprichardson Should this added to the v1.0.0 milestone?

Ideally yes, but if it's going to be a lot of work, no.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 28, 2016

OK, should document this though.

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 17, 2016

Fixed by #324.

@RyanZim RyanZim closed this as completed Dec 17, 2016
nicks pushed a commit to Medium/node-fs-extra that referenced this issue Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants