Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Aug 21, 2017

This was now a compile error thanks to microsoft/TypeScript#17660. copy takes an options object, not a function. (https://github.com/jprichardson/node-fs-extra/blob/master/docs/copy.md)

@typescript-bot typescript-bot added the Unowned This PR touches a package that doesn't have any listed owners. label Aug 21, 2017
@DanielRosenwasser
Copy link
Member

Looks like docs imply the callback is a valid 3rd argument.

@ghost
Copy link
Author

ghost commented Aug 21, 2017

Hm, but the callback takes an error, while in the tests it is shown taking a string. So it may have been intended to be a filter instead.
Waiting for response from authors: @alan-agius4 @midknight41 @shiftkey @mees-

@alan-agius4
Copy link
Contributor

alan-agius4 commented Aug 21, 2017

Hi @Andy-MS, I agree here that it seems that the tests were wrong in the first place.

Maybe we can indicate that in the callback the error can be undefined such as callback: (err: Error | undefined) => void

I also noticed something by checking the docs, that the filter signature itself is wrong as it seems that it accepts src and dest as params.

https://github.com/jprichardson/node-fs-extra/blob/master/docs/copy.md

const fs = require('fs-extra')

const filterFunc = (src, dest) => {
  // your logic here
  // it will be copied if return true
}

fs.copy('/tmp/mydir', '/tmp/mynewdir', { filter: filterFunc }, err => {
  if (err) return console.error(err)

  console.log('success!')
})

But for all these we can create a separate PR.

No objection in merging this from my end! 👍

@ghost
Copy link
Author

ghost commented Aug 21, 2017

@alan-agius4 Thanks, please create a PR for those changes too.

@ghost ghost merged commit 3fcde35 into master Aug 21, 2017
@ghost ghost deleted the fs-extra_fix branch August 21, 2017 20:39
@alan-agius4
Copy link
Contributor

alan-agius4 commented Aug 21, 2017

@Andy-MS I'll try to find some time in the coming days :), thanks for your PR

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Unowned This PR touches a package that doesn't have any listed owners.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants