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

Rewrite lib/copy/ncp.js #292

Closed
RyanZim opened this issue Oct 26, 2016 · 18 comments
Closed

Rewrite lib/copy/ncp.js #292

RyanZim opened this issue Oct 26, 2016 · 18 comments

Comments

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 26, 2016

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 RyanZim added this to the 1.0.0 milestone Oct 26, 2016
@RyanZim RyanZim self-assigned this Oct 26, 2016
@jprichardson
Copy link
Owner

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 RyanZim removed this from the 1.0.0 milestone Oct 27, 2016
@RyanZim
Copy link
Collaborator Author

RyanZim commented Oct 27, 2016

See #294

@RyanZim
Copy link
Collaborator Author

RyanZim commented Feb 3, 2017

@jprichardson Refs #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
Copy link
Collaborator

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?

@RyanZim RyanZim mentioned this issue Feb 18, 2017
@jprichardson
Copy link
Owner

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
Copy link
Collaborator

So @RyanZim how much are you progressed on rewriting or refactoring or whatever you wanna call it 😄 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?! 😃

@RyanZim
Copy link
Collaborator Author

RyanZim commented Feb 18, 2017

@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. 😄

I haven't gotten far with my implementation.

@manidlou
Copy link
Collaborator

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
Copy link
Collaborator Author

RyanZim commented Feb 25, 2017

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
Copy link
Collaborator

@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 👌! 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! 😲!

@manidlou
Copy link
Collaborator

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
Copy link
Collaborator Author

RyanZim commented Feb 25, 2017

Oh man! what a weird cases!

👍

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
Copy link
Collaborator

manidlou commented Sep 1, 2017

@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
Copy link
Owner

@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
Copy link
Collaborator Author

RyanZim commented Sep 5, 2017

@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
Copy link
Collaborator

manidlou commented Sep 6, 2017

@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. 👍

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 RyanZim added this to the 5.0.0 milestone Nov 9, 2017
@RyanZim
Copy link
Collaborator Author

RyanZim commented Dec 11, 2017

Fixed in 5.0.0 🎉

@RyanZim RyanZim closed this as completed Dec 11, 2017
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

3 participants