fs: add fs.copy, fs.copySync, fs.delete and fs.deleteSync with docs and tests. #12583

JOT85 · 2017-04-22T08:23:03Z

So I and many other often need to recursively copy or delete
directory's with node and copy file that keep their permissions and
properties etc. Many use an external library or write their own and
that takes up a lot of time and adds deps to your project which you
might not want.

I have added fs.copy and fs.copySync to recursively copy directory's or
just copy files, all files/directory's copied with fs.copy and
fs.copySync retain they owners, mode and times.

fs.delete and fs.deleteSync either delete a file (basically an alias
for fs.unlink) or of a directory is passed into the path recursively
remove the content of the directory and remove the directory itself.

There are however a few notes, this is writen in JavaScript only and
thus calls other functions in the fs module, I have placed the code at
the bottom of lib/fs.js however there may be a better place to put the
code.

I also change open the directory's for copying in different modes
dependent on the platform, I use 'process.platform==="win32"?"r+":"r"'
to determine the mode to open the directory in, I have done this
because I found during testing that on Windows, you need to have the
directory open in r+ mode to modify it's properties whereas on Linux it
throws when you attempt to open a directory in r+ mode & you can open
it in r mode and still change the properties. I don't have a Mac OS
computer & thus cannot test it on Mac so this mode could be wrong on
Mac.

I also use read streams and write streams in fs.copy however I use
fs.readFileSync to read the file in fs.copySync, I know that this could
load the entire file into the main memory and thus not be a great way
of doing it however I am not sure how to do this better. So advise on
this would be helpful.

I have also used readable.on('end') to determine when the copying of
the file contents has ended, however I have seen the finish event used
in some other cases, the end event seems to work to I haven't changed
it but again that is something that might need changing.

My tests are relatively simplistic however they do test the copying. I
haven't got a test for the overwrite mode being a function however I
can write that if it is needed.

Hope this is OK,
Jacob

Checklist

make -j4 test (UNIX), or vcbuild test (Windows passes, but I cannot test on linux)
documentation is changed or added
commit message follows [commit guidelines] (https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-guidelines)

Affected core subsystem(s)

It changes the fs module, and adds docs.

…nd tests. So I and many other often need to recursively copy or delete directory's with node and copy file that keep their permissions and properties etc. Many use an external library or write their own and that takes up a lot of time and adds deps to your project which you might not want. I have added fs.copy and fs.copySync to recursively copy directory's or just copy files, all files/directory's copied with fs.copy and fs.copySync retain they owners, mode and times. fs.delete and fs.deleteSync either delete a file (basically an alias for fs.unlink) or of a directory is passed into the path recursively remove the content of the directory and remove the directory itself. There are however a few notes, this is writen in JavaScript only and thus calls other functions in the fs module, I have placed the code at the bottom of lib/fs.js however there may be a better place to put the code. I also change open the directory's for copying in different modes dependent on the platform, I use 'process.platform==="win32"?"r+":"r"' to determine the mode to open the directory in, I have done this because I found during testing that on Windows, you need to have the directory open in r+ mode to modify it's properties whereas on Linux it throws when you attempt to open a directory in r+ mode & you can open it in r mode and still change the properties. I don't have a Mac OS computer & thus cannot test it on Mac so this mode could be wrong on Mac. I also use read streams and write streams in fs.copy however I use fs.readFileSync to read the file in fs.copySync, I know that this could load the entire file into the main memory and thus not be a great way of doing it however I am not sure how to do this better. So advise on this would be helpful. I have also used readable.on('end') to determine when the copying of the file contents has ended, however I have seen the finish event used in some other cases, the end event seems to work to I haven't changed it but again that is something that might need changing. My tests are relatively simplistic however they do test the copying. I haven't got a test for the overwrite mode being a function however I can write that if it is needed. Hope this is OK, Jacob

spelling from us to is and included the default value for overwrite_mode.

jseijas · 2017-04-22T12:44:41Z

Perhaps you can pass ``make -j4 test```. The linter is giving 481 problems. Also, try to follow the style that the code already have. Use spacing 2 instead of tabs.

vsemozhetbyt · 2017-04-22T12:53:09Z

@JOT85 Thank you for the contribution!

@jseijas is right. We can't even run the CI tests before linting problems are addressed. Please, analyze the Node.js code base style, try to use ESLint for your code with Node.js linting rules.

tniessen · 2017-04-22T13:44:48Z

Is it actually desirable to include this in the node.js core, considering that it does not contain native bindings and the overhead of npm dependencies is relatively small?

use an external library or write their own and that takes up a lot of time and adds deps to your project which you might not want.

According to this logic, we should integrate all of the "Most depended-upon packages" from npm into node itself.

refack · 2017-04-22T14:05:31Z

According to this logic, we should integrate all of the "Most depended-upon packages" from npm into node itself.

Not a bad idea 😉.
IMHO it's something to consider.
Good candidates are stable, mature, and ubiquitous modules (fs-extra, debug, rimraf, etc) where core can give and get some obvious benefit.
I think express or mocha for instance have a life of their own, and are counter examples.

refack · 2017-04-22T14:08:31Z

Is it actually desirable to include this in the node.js core, considering that it does not contain native bindings...

@JOT85 this is a valid point. Do you think this PR can be improved by using native bindings?

cjihrig · 2017-04-22T14:12:51Z

Style issues aside, I don't think this needs to be in core. Please just use something like fs-extra, or publish your own module.

mscdex · 2017-04-22T18:16:42Z

-1 on this. I agree this is better to have outside of core because it's not terribly difficult to do and the implementation of such functions is/could be quite opinionated.

Fishrock123 · 2017-04-22T19:02:02Z

According to this logic, we should integrate all of the "Most depended-upon packages" from npm into node itself.

This isn't really a great idea though -- it puts an unreasonable burden on core to maintain more and more. These are not difficult to do outside of core, so they should belong outside of core.

It could be conceivable that we may want them if they pose a large barrier to newcomers. However, we could also point out modules in that case that people could use for that functionality.

Generally, we should actually be integrating some things, but typically only those that are particularly painful to do outside of core. :)

Fishrock123 · 2017-04-22T19:03:33Z

(I see this is you're first PR! thanks for including everything, including documentation; hopefully you'll continue to contribute, even if this doesn't make it in.)

tniessen · 2017-04-22T19:06:02Z

According to this logic, we should integrate all of the "Most depended-upon packages" from npm into node itself.

This isn't really a great idea though

I am sorry if I was not clear about this part, I oppose this idea, I do not support it.

JOT85 · 2017-04-22T22:38:36Z

Hi, sorry for the late response, I have been busy all day.
I will look over the linting issues tomorrow as I don't have time now, I will also correct the tabs, sorry for wasting time, I will sort it out as soon as I can.
Thanks for the feedback.

refack · 2017-04-22T22:42:02Z

@JOT85 I like this, and I think it makes allot of sense, but try and think if some native binding could make this better, more organic to incorporate. If it's a pure js solution then it should live as an independent module...

refack · 2017-04-22T22:44:31Z

sorry for wasting time

I think I speak for most people here; we are understanding of rookie mistakes, and we appreciate your effort. We do not consider this a waste of time!

jseijas · 2017-04-22T22:52:19Z

It's never a waste of time ^.^

JOT85 · 2017-04-23T08:14:46Z

OK, so I have sorted out the tabs and replaced them all with 2 spaces. Sorry about that.
I have also fixed all the linting errors. There were some silly things such as the 80 character limit per line that required me to write some messy code to fit it in but other then that it is better now.
Thanks for the advice though because there was an error that I corrected so that saved a lot of time :)

vsemozhetbyt · 2017-04-23T09:07:01Z

@JOT85 You can lint your files with this commands from repository root folder:

node tools/eslint/bin/eslint.js --rulesdir tools/eslint-rules/ lib/fs.js

node tools/eslint/bin/eslint.js --rulesdir tools/eslint-rules/ test/parallel/test-fs-copy-and-delete.js

Currently, the logs are:

fs.js
  2941:10  error  Use of `let` as the loop variable in a for-loop is not recommended. Please use `var` instead  no-let-in-for-declaration

test-fs-copy-and-delete.js
   82:5  error  Use assert.ifError(err) instead  prefer-assert-iferror
  119:9  error  Use assert.ifError(err) instead  prefer-assert-iferror

…stead of throw in test-fs-copy-and-delete.js

JOT85 · 2017-04-23T09:13:28Z

Thanks for that, I was just using vscode... :(
Sorted now. :)

vsemozhetbyt · 2017-04-23T09:25:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7609/

Maybe this will be helpful for you even if this PR is not approved and you want to make an npm module.

vsemozhetbyt · 2017-04-23T09:31:03Z

From the first failed build:

not ok 360 parallel/test-fs-copy-and-delete
  ---
  duration_ms: 0.73
  severity: fail
  stack: |-
    fs.js:871
      return binding.mkdir(pathModule._makeLong(path),
                     ^
    
    Error: ENOENT: no such file or directory, mkdir '/data/iojs/node-tmp/tmp.2/copyanddeletetest/test1/test1'
        at Object.fs.mkdirSync (fs.js:871:18)
        at setUpDir (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-fs-copy-and-delete.js:11:6)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-fs-copy-and-delete.js:22:1)
        at Module._compile (module.js:582:30)
        at Object.Module._extensions..js (module.js:593:10)
        at Module.load (module.js:516:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
        at Function.Module.runMain (module.js:618:10)
        at startup (bootstrap_node.js:144:16)

JOT85 · 2017-04-23T09:37:45Z

I think the issue was actually with my test code, I never created the directory to run the tests in.
Sorry, it should be fixed now, would you mind having a quick look over it please.

vsemozhetbyt · 2017-04-23T09:43:03Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7610/

vsemozhetbyt · 2017-04-23T09:48:10Z

Still something wrong)

not ok 361 parallel/test-fs-copy-and-delete
  ---
  duration_ms: 0.71
  severity: fail
  stack: |-
    fs.js:871
      return binding.mkdir(pathModule._makeLong(path),
                     ^
    
    Error: ENOENT: no such file or directory, mkdir '/data/iojs/node-tmp/tmp.3/copyanddeletetest/test1'
        at Object.fs.mkdirSync (fs.js:871:18)
        at setUpDir (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-fs-copy-and-delete.js:12:8)
        at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-fs-copy-and-delete.js:25:1)
        at Module._compile (module.js:582:30)
        at Object.Module._extensions..js (module.js:593:10)
        at Module.load (module.js:516:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
        at Function.Module.runMain (module.js:618:10)
        at startup (bootstrap_node.js:144:16)

…hat they are on aix.

gibfahn · 2017-04-23T15:15:13Z

@JOT85 output on AIX:

> process.platform
'aix'

JOT85 · 2017-04-23T15:15:33Z

See how that runs on aix, now everything passes except aix, so the tests now log process.platform so I can figure out what it is.

JOT85 · 2017-04-23T15:16:15Z

@gibfahn Thanks! No need for that last commit then... :(

JOT85 · 2017-04-23T15:19:29Z

Cool, fingers crossed this works.
Also @gibfahn do you know if there are any other fs functions (other than fs.futimes) that don't work on aix?

vsemozhetbyt · 2017-04-23T15:22:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7617/

gibfahn · 2017-04-23T15:22:29Z

I'm -1 on this for the reasons mentioned above. Basically having a small core allows us to provide a consistent and stable platform for others to build upon. If it can just as well be implemented outside, then the problems outweigh the benefits.

Some links on the subject:
https://strongloop.com/strongblog/the-future-of-node-podcast-series-small-core-keeping-node-core-small/
https://medium.com/node-js-javascript/make-node-js-core-bigger-97ca7ef62b77
https://vimeo.com/56402326
#7098
https://r.va.gg/2014/06/why-i-dont-use-nodes-core-stream-module.html

cc/ @sam-github

gibfahn · 2017-04-23T15:25:50Z

Also @gibfahn do you know if there are any other fs functions (other than fs.futimes) that don't work on aix?

If something doesn't work it should be in the docs: https://nodejs.org/api/fs.html

If not that's a documentation issue and we should fix it. So no, nothing else that I'm aware of.

JOT85 · 2017-04-23T15:28:53Z

@gibfahn thanks.
I have taken your point on board and see where you are coming from, however I personally (from my much lower down position compared to you and not having anywhere near your experience) disagree for reasons that I have stated previously.
As for the fs docs you could fix it yourself but if you would like I will happily create another pull request for it.

JOT85 · 2017-04-23T15:33:28Z

Hmm, by the looks of it the aix test that failed wasn't on the latest commit that fixes the issue.

JOT85 · 2017-04-23T15:39:17Z

Also, the windows fail wasn't caused by this PR.

for any other platform when it should have been the other way round.

JOT85 · 2017-04-23T15:48:29Z

All the fails are because I did things the opposite way round with dirs in fs.copySync. Fixed now.
Sorry

And yeah, I only push that now because I entered the wrong password and didn't check... :(

JOT85 · 2017-04-23T16:05:37Z

@vsemozhetbyt or someone else can you please run the tests with the latest commit, I think everything is good now :)

vsemozhetbyt · 2017-04-23T16:15:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7618/

gibfahn · 2017-04-23T16:25:31Z

Many use an external library or write their own and
that takes up a lot of time and adds deps to your project which you
might not want.

Out of interest, why don't you want deps added to a project (but are happy with the same deps being added to every Node.js project by being bundled into Node)?

JOT85 · 2017-04-23T16:38:47Z

@gibfahn I have a few reasons for this.
For a start I personally don't like relying on other peoples code - I understand that obviously node isn't written by myself the the process to add code requires it to be of a good standard and work well, which lowers the chance of something going wrong. This is because (as I have explained already that node isn't) it could be unreliable and although large libraries obviously will be because of the large user-base smaller ones might not be.
And, I also like to understand how everything works. I know that most sane people are not like this however I like to read through projects like node & make sure that I understand how everything that I am using works mainly for education purposes. And obviously this can also be done with external libraries, they often come with unwanted features, that can lead to even more problems.
Also, on the subject of problems, having something within node itself means it is very unlikely to clash with anything else, which some libraries do.
I also have had past experiences where I used only 2 or 3 modules and yet had over 50 more deps, which could lead to licencing problems or unexpected problem running your own code caused by deps you didn't know you had & don't understand (which as I already said I personalty don't like not understanding :(

Just to make this clear, I am not against deps at all I just don't like having to many of them.

JOT85 · 2017-04-23T17:19:45Z

Couple more things, firstly, the previous windows test was 'unstable' and that wasn't down to test-fs-copy-and-delete, it was down to something else.
Also, the previous commit adds some timing to make sure that the mtimes cannot be similar via a fluke in the tests.
Can some more tests please be run on this @vsemozhetbyt (or anyone else who can)
Thanks

gibfahn · 2017-04-23T17:21:53Z

CI 8: https://ci.nodejs.org/job/node-test-commit/9363/

cjihrig · 2017-04-23T17:47:50Z

Given that the collaborators thus far have been pretty -1 on this, I think we should close the issue and stop running CI jobs on it. Or at least, we could come to a decision of whether or not we're going to take the PR.

I totally get the not wanting dependencies written by other people thing. But it looks like the code is mostly done, so you could just publish your own module.

tniessen · 2017-04-23T17:49:22Z

Just to throw in my opinion:

I think composite operations (such as recursive copies and deletions) pose new challenges. For example, what if you wanted to report the progress of a recursive operation to a user? What if something fails within a recursive operation, how do you handle that error? Can you resume recursive copy operations if they fail? Can you selectively copy directories? Can you determine the progress of a file copying operation?

Another thing to consider is that there are existing modules for these operations, some of which even allow to use arbitrary fs implementations, e.g. graceful-fs. Some modules even allow abstraction to a point at which FTP etc. could be used with the same implementation of the operation.

This is because (as I have explained already that node isn't) it could be unreliable and although large libraries obviously will be because of the large user-base smaller ones might not be.

We are talking about very common features only, I think it is safe to ignore "smaller ones". If an npm module does not have a "large user-base", it should not even be considered for inclusion in node.

And obviously this can also be done with external libraries, they often come with unwanted features, that can lead to even more problems.

So does node, if you don't need the crypto module, you could say it comes with "unwanted features". Yet I don't see a problem with that.

Also, on the subject of problems, having something within node itself means it is very unlikely to clash with anything else, which some libraries do.

No module should have side effects apart from those explicitely required. Side effects are usually considered bad and most popular modules refrain from having any.

I also have had past experiences where I used only 2 or 3 modules and yet had over 50 more deps, which could lead to licencing problems or unexpected problem running your own code caused by deps you didn't know you had & don't understand

Licensing is a fair point, but I guess if a problem is popular enough to be included in the node core, there will be an npm module doing the same thing with a compatible license (fs-extra in this case).

If these functions are as common as you say they are, I am sure there are well-tested existing libraries that do the job as good as possible within JavaScript, maybe even with native bindings.

JOT85 · 2017-04-23T18:16:45Z

Well, the code is there, the docs are there, the test scripts are mostly there (however more could be written).
All the tests pass so yeah, the code is pretty much done.
@tniessen I understand all your points & agree with them mostly, however I also think that having even a simple version of it in node itself will cover the majority of use cases so could be very useful.
I will now wait as @cjihrig suggested for a decision.
I would just like to quickly thank everyone for their advice and effort (especially @vsemozhetbyt, @gibfahn). Let's see what happens & what decision is made, either way this has been a great learning experience :)
Thanks

evanlucas · 2017-04-24T11:48:53Z

Although we greatly appreciate the effort @JOT85, I'm also -1 on this. I think this should be published in a package on npm, not included in core.

jasnell · 2017-04-24T20:12:47Z

I'm also -1. I definitely do appreciate the work, but these are functions that are best left to userland. Given the -1's, I'm closing.

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Apr 22, 2017

Ammended docs: enclosed overwrite_mode in square brackets, corrected

3a2ef1d

spelling from us to is and included the default value for overwrite_mode.

vsemozhetbyt added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 22, 2017

Replaced tabs with spaces and fixed linting issues.

4482021

Remove directory after test

39b0e12

Use var instead of let in for loop in fs.js and use assert.ifError in…

45296d6

…stead of throw in test-fs-copy-and-delete.js

Make the test dir if it doesn't already exist.

050a6ee

Log process.platform and os.platform() in test scripts to determine w…

3f6306a

…hat they are on aix.

Don't call fs.futimes on aix platform.

a2a6648

I made a stupid mistake and triggered fs.futimesSync for AIX and not

ea1db2d

for any other platform when it should have been the other way round.

gibfahn mentioned this pull request Apr 23, 2017

Document that fs.futimes isn't supported on AIX #12609

Closed

Made sure that the test cannot fluke with timings.

bad81e7

mhdawson requested review from jasnell and removed request for jasnell April 24, 2017 13:12

jasnell closed this Apr 24, 2017

fs: add fs.copy, fs.copySync, fs.delete and fs.deleteSync with docs and tests. #12583

fs: add fs.copy, fs.copySync, fs.delete and fs.deleteSync with docs and tests. #12583

Conversation

JOT85 commented Apr 22, 2017 • edited Loading

Checklist

Affected core subsystem(s)

jseijas commented Apr 22, 2017 • edited Loading

vsemozhetbyt commented Apr 22, 2017 • edited Loading

tniessen commented Apr 22, 2017

refack commented Apr 22, 2017 • edited Loading

refack commented Apr 22, 2017

cjihrig commented Apr 22, 2017

mscdex commented Apr 22, 2017 • edited Loading

Fishrock123 commented Apr 22, 2017

Fishrock123 commented Apr 22, 2017

tniessen commented Apr 22, 2017

JOT85 commented Apr 22, 2017

refack commented Apr 22, 2017

refack commented Apr 22, 2017

jseijas commented Apr 22, 2017

JOT85 commented Apr 23, 2017

vsemozhetbyt commented Apr 23, 2017 • edited Loading

JOT85 commented Apr 23, 2017

vsemozhetbyt commented Apr 23, 2017

vsemozhetbyt commented Apr 23, 2017

JOT85 commented Apr 23, 2017

vsemozhetbyt commented Apr 23, 2017

vsemozhetbyt commented Apr 23, 2017

gibfahn commented Apr 23, 2017

JOT85 commented Apr 23, 2017

JOT85 commented Apr 23, 2017

JOT85 commented Apr 23, 2017

vsemozhetbyt commented Apr 23, 2017

gibfahn commented Apr 23, 2017

gibfahn commented Apr 23, 2017

JOT85 commented Apr 23, 2017 • edited Loading

JOT85 commented Apr 23, 2017

JOT85 commented Apr 23, 2017

JOT85 commented Apr 23, 2017 • edited Loading

JOT85 commented Apr 23, 2017 • edited Loading

vsemozhetbyt commented Apr 23, 2017

gibfahn commented Apr 23, 2017

JOT85 commented Apr 23, 2017

JOT85 commented Apr 23, 2017 • edited Loading

gibfahn commented Apr 23, 2017

cjihrig commented Apr 23, 2017 • edited Loading

tniessen commented Apr 23, 2017

JOT85 commented Apr 23, 2017 • edited Loading

evanlucas commented Apr 24, 2017

jasnell commented Apr 24, 2017

JOT85 commented Apr 22, 2017 •

edited

Loading

jseijas commented Apr 22, 2017 •

edited

Loading

vsemozhetbyt commented Apr 22, 2017 •

edited

Loading

refack commented Apr 22, 2017 •

edited

Loading

mscdex commented Apr 22, 2017 •

edited

Loading

vsemozhetbyt commented Apr 23, 2017 •

edited

Loading

JOT85 commented Apr 23, 2017 •

edited

Loading

JOT85 commented Apr 23, 2017 •

edited

Loading

JOT85 commented Apr 23, 2017 •

edited

Loading

JOT85 commented Apr 23, 2017 •

edited

Loading

cjihrig commented Apr 23, 2017 •

edited

Loading

JOT85 commented Apr 23, 2017 •

edited

Loading