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

Use fs.copyFile where applicable #490

Closed
benjamingr opened this issue Sep 12, 2017 · 13 comments
Closed

Use fs.copyFile where applicable #490

benjamingr opened this issue Sep 12, 2017 · 13 comments

Comments

@benjamingr
Copy link

fs.copyFile has landed with Node.js 8.5.0. You should consider using it for copying files when/where applicable.

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 12, 2017

@benjamingr Thanks for bringing this up. Man, I'm so happy to have this in core. 🎉

One question I have: How does copyFile handle symlinks?


Todo list:

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 12, 2017

Self-assigning for now to get the ball rolling, may ask others to help later though.

RyanZim added a commit that referenced this issue Sep 12, 2017
RyanZim added a commit that referenced this issue Sep 12, 2017
@manidlou
Copy link
Collaborator

One question I have: How does copyFile handle symlinks?

@RyanZim I tested when src is a symlink, and copyFile() creates a regular file for dest.

@metabench
Copy link

I'm looking at this project as a way to preserve timestamps while copying files (in Windows), but have run into the EPERM error while copying symlinks. Please be wary of using another method that copies the symlinks but does not preserve timestamps if it's an option to do so.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 27, 2017

@manidlou Do you want to work on using copyFile where it's supported, falling back to our current implementation in older versions of Node? The work should be based against the develop branch.

@manidlou
Copy link
Collaborator

manidlou commented Oct 27, 2017

@manidlou Do you want to work on using copyFile where it's supported, falling back to our current implementation in older versions of Node? The work should be based against the develop branch.

Sure thing. I'll work on it.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 31, 2017

PR done for copy: #505

@manidlou We also need to use fs.copyFileSync in copySync when possible.

@manidlou
Copy link
Collaborator

manidlou commented Nov 1, 2017

@manidlou We also need to use fs.copyFileSync in copySync when possible.

That's right! I am working on it.

@manidlou
Copy link
Collaborator

manidlou commented Nov 1, 2017

It's just copySync needs serious cleanup (its implementation is not consistent with new changes that we applied to copy), and I am willing to work on it. But, I think it's better to add the native fs.copyFileSync first, then work on other parts of it! let me know if there is any objections!

@manidlou
Copy link
Collaborator

manidlou commented Nov 1, 2017

PR updated for copySync: #505.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 7, 2017

OK, I think everything that needs done here is done in develop; closing out for now; will be released in the next major version.

@RyanZim RyanZim closed this as completed Nov 7, 2017
@arcanis
Copy link
Contributor

arcanis commented Nov 28, 2017

@RyanZim By any chance, do you have an idea when the next version will be released? The copy rewrite listed in the 5.0.0 milestones seems to have been merged in #502, so is "cannot copy a directory within itself" the last missing part?

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 28, 2017

@arcanis AFAIK, everything's ready for the release, I'd have to double-check. I've just been too busy to release it. I'll try to hit it soon, though it doesn't suit me tonight. Ping me if I don't have 5.0.0 out within a week.

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

5 participants