Node-fs-extra: Rewrite lib/copy/ncp.js

[RyanZim]

This needs rewritten. It has several bugs and is overly complex. It was written for use with fs instead of graceful-fs, and contains a lot of concurrency-limiting code that is now unnecessary.

Once rewritten, it may be small enough to inline in lib/copy/copy.js; talk about that later.

I'll try to do this myself, but help welcome. Adding this to the v1.0.0 milestone, but may have to be pushed back to v2.0.0.

X-Refs: #233.

[RyanZim]

@jprichardson Found why we need https://github.com/jprichardson/node-fs-extra/blob/master/lib/copy/ncp.js#L150: http://stackoverflow.com/questions/34720794/create-directory-with-permission-in-node-js.

Advice here?

[jprichardson]

I think that rewriting is a mistake for 1.0.0 and shouldn't even be considered even though it's bad legacy code. By all means, don't let me stop you, it's just in my long experience of writing code a rewrite is a bad idea 99% of the time.

[RyanZim]

See #294

[RyanZim]

@jprichardson Refs https://github.com/jprichardson/node-fs-extra/issues/83. This needs done sometime. (don't know when)

Would you consider a custom build/refactor based on https://github.com/davglass/cpr sometime in the future? Just a random idea, since it's one of the few well-maintained directory copiers.

Or do you want to roll your own, based on klaw?

[manidlou]

So what is the decision on this? We need to decide what we want to do about this as ncp.js is the source of a lot of issues. I like @RyanZim idea about using native promise and avoid using counters.

@jprichardson what should we do about this?

[jprichardson]

Would you consider a custom build/refactor based on https://github.com/davglass/cpr sometime in the future? Just a random idea, since it's one of the few well-maintained directory copiers.

Or do you want to roll your own, based on klaw?

I don't care much what it's based on at the moment, I just think whatever we need to do to get it to fulfill its implicit contract (re: work) is just fine by me! 😊

[manidlou]

So @RyanZim how much are you progressed on rewriting or refactoring or whatever you wanna call it :smile: of copy()?! I rolled one using the good parts of what we've had already and replacing the bad parts such as counters with new stuff like native Promise.all() along with Array.map() to achieve better concurrency control.

I know you and @jprichardson are busy but I'd like to know your thoughts on that see if that is in the right direction. Should I create a branch here or in my forked repo?! :smiley:

[RyanZim]

@manidlou I don't care if the branch is here or in a fork; please do open a WIP PR, so we can see what you're up to. :smile:

I haven't gotten far with my implementation.

[manidlou]

Although it may be a rare case to happen but I believe we should prepare for this.

In general, how should copy() behave when src is a subdirectory of dest?

For example, copy('dest/nested/src', 'dest', cb). As we see, it is not copying directory into itself rather it is copying to some higher parent directory.

What I think is copying should occur without a problem (or at least after some sanity checks). However, with the current algorithm, since options.overwrite defaults true and we remove dest before copying, therefore we end up removing src contents as well!

So, @jprichardson @RyanZim what is the decision on this?

[RyanZim]

since options.overwrite defaults true and we remove dest before copying, therefore we end up removing src contents as well!

@manidlou We don't actually remove the dest folder before copying, we just remove files in dest that have the same name as files in src.

[manidlou]

@manidlou We don't actually remove the dest folder before copying, we just remove files in dest that have the same name as files in src.

ohh! that makes sense! right action :ok_hand:! So, I assume it should be the same for symlinks as well, right? like when for example src is a symlink and dest is a directory that src resolved path is a subdirectory of dest. Oh man! what a weird cases! :astonished:!

[manidlou]

Also is this why we do this https://github.com/jprichardson/node-fs-extra/blob/master/lib/copy/ncp.js#L79 (replacing currentPath with targetPath)?

[RyanZim]

Oh man! what a weird cases!

:+1:

So, I assume it should be the same for symlinks as well, right?

I think so, if I'm following you.

Also is this why we do this https://github.com/jprichardson/node-fs-extra/blob/master/lib/copy/ncp.js#L79 (replacing currentPath with targetPath)?

file.name is an absolute path that contains the source directory. We replace the source directory portion of the path with the dest directory, to obtain the target path.

[manidlou]

@jprichardson I found some old comments that you brought the idea of rewriting copy on top of https://github.com/jprichardson/node-klaw (#196). Just wondering what happened to that idea? Is it still a favorable alternative?

[jprichardson]

@manidlou it seemed to make sense to me at the time. As I remember though, @RyanZim may have had some objections, but I don't recall for sure. I'll be talking to @RyanZim on Saturday or Monday to check.

[RyanZim]

@manidlou Sorry for being so long in getting back here, and sorry for being unresponsive in general. I want to try and do better on responding quickly so you can move forward.

Overall, I like your changes in #374. You did a great job adding a ton of tests. The reason it hasn't been merged yet is twofold:

1. It's a huge rewrite, regressions are very possible. This alone shouldn't stop us from merging it. I am planning on releasing the copy changes in a major release with no other changes in it, that way we can tell people to downgrade if there is a bug.
2. I'm not happy with the filter option how it currently stands, or after your changes.

I talked with @jprichardson about filter, and here's what we came up with:

• Recursing on a failed filter is counter-intuitive, and can be a huge perf hit.
• Trying to copy all .js files using the current method is very difficult. We need to fix this.

It'd be best if you could return Promises to filter. This would allow you to stat, readDir, etc. inside the filter function, without using sync methods. Of course, returning simple boolean values would also be supported.

I'm not asking you to implement these changes, I'm willing to do that myself. But I'd like you to change your PR to remove the noRecurseOnFailedFilter option and have the filter _not recurse_ if it fails.

After that, I'll merge your PR and get the async filter implemented, then it's :shipit: ! Let me know if you feel strongly otherwise.

[manidlou]

@RyanZim no need to be sorry! I know you are very busy and I totally understand and appreciate how hard you try to keep everything here live and up to date.

I am planning on releasing the copy changes in a major release with no other changes in it, that way we can tell people to downgrade if there is a bug. :+1:

Agreed on the performance drawback of changes on filter.

But I'd like you to change your PR to remove the noRecurseOnFailedFilter option and have the filter not recurse if it fails.

Sure thing.

[RyanZim]

Fixed in 5.0.0 :tada: