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

Add cloning support #40

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ node_modules
yarn.lock
.nyc_output
coverage
ts-build
6 changes: 6 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,13 @@ os:
- windows
language: node_js
node_js:
- '14'
- '12'
- '10'
before_install:
# Run a one-liner that prints "mount" information about the current dir
- if command -v mount grep df head tail cut &>/dev/null; then mount | grep "^$(df -Pk . | head -n 2 | tail -n 1 | cut -f 1 -d ' ') "; fi
Artoria2e5 marked this conversation as resolved.
Show resolved Hide resolved
# The cache got stuck at some old version that satisfies the requirements but breaks CI...
- npm update --depth 0 --no-save
after_success:
- './node_modules/.bin/nyc report --reporter=text-lcov | ./node_modules/.bin/coveralls'
5 changes: 5 additions & 0 deletions cp-file-error.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
/// <reference path="index.d.ts" />
'use strict';
const NestedError = require('nested-error-stacks');

class CpFileError extends NestedError {
/**
* @param {string} message
* @param {Error} [nested]
*/
constructor(message, nested) {
super(message, nested);
Object.assign(this, nested);
Expand Down
54 changes: 54 additions & 0 deletions fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const fs = require('graceful-fs');
const makeDir = require('make-dir');
const pEvent = require('p-event');
const CpFileError = require('./cp-file-error');
const {version} = process;

const stat = promisify(fs.stat);
const lstat = promisify(fs.lstat);
Expand All @@ -13,6 +14,10 @@ const chmod = promisify(fs.chmod);
exports.closeSync = fs.closeSync.bind(fs);
exports.createWriteStream = fs.createWriteStream.bind(fs);

/**
@param {import('fs').PathLike} path
@param {object} [options]
*/
exports.createReadStream = async (path, options) => {
const read = fs.createReadStream(path, options);

Expand All @@ -25,22 +30,40 @@ exports.createReadStream = async (path, options) => {
return read;
};

/**
@param {import('fs').PathLike} path
*/
exports.stat = path => stat(path).catch(error => {
throw new CpFileError(`Cannot stat path \`${path}\`: ${error.message}`, error);
});

/**
@param {import('fs').PathLike} path
*/
exports.lstat = path => lstat(path).catch(error => {
throw new CpFileError(`lstat \`${path}\` failed: ${error.message}`, error);
});

/**
@param {import('fs').PathLike} path
@param {string | number | Date} atime
@param {string | number | Date} mtime
*/
exports.utimes = (path, atime, mtime) => utimes(path, atime, mtime).catch(error => {
throw new CpFileError(`utimes \`${path}\` failed: ${error.message}`, error);
});

/**
@param {import('fs').PathLike} path
@param {import('fs').Mode} mode
*/
exports.chmod = (path, mode) => chmod(path, mode).catch(error => {
throw new CpFileError(`chmod \`${path}\` failed: ${error.message}`, error);
});

/**
@param {import('fs').PathLike} path
*/
exports.statSync = path => {
try {
return fs.statSync(path);
Expand All @@ -49,6 +72,11 @@ exports.statSync = path => {
}
};

/**
@param {import('fs').PathLike} path
@param {string | number | Date} atime
@param {string | number | Date} mtime
*/
exports.utimesSync = (path, atime, mtime) => {
try {
return fs.utimesSync(path, atime, mtime);
Expand All @@ -57,10 +85,16 @@ exports.utimesSync = (path, atime, mtime) => {
}
};

/**
@param {string} path
*/
exports.makeDir = path => makeDir(path, {fs}).catch(error => {
throw new CpFileError(`Cannot create directory \`${path}\`: ${error.message}`, error);
});

/**
@param {string} path
*/
exports.makeDirSync = path => {
try {
makeDir.sync(path, {fs});
Expand All @@ -69,8 +103,28 @@ exports.makeDirSync = path => {
}
};

/**
@param {import('fs').PathLike} source
@param {import('fs').PathLike} destination
@param {number?} [flags]
*/
exports.cloneFileSync = (source, destination, flags) => {
try {
if (flags == undefined) flags = 0;
fs.copyFileSync(source, destination, flags | fs.constants.COPYFILE_FICLONE_FORCE);
} catch (error) {
throw new CpFileError(`Cannot clone from \`${source}\` to \`${destination}\`: ${error.message}`, error);
}
};

/**
@param {import('fs').PathLike} source
@param {import('fs').PathLike} destination
@param {number?} [flags]
*/
exports.copyFileSync = (source, destination, flags) => {
try {
if (flags === null) flags = undefined;
fs.copyFileSync(source, destination, flags);
} catch (error) {
throw new CpFileError(`Cannot copy from \`${source}\` to \`${destination}\`: ${error.message}`, error);
Expand Down
7 changes: 7 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,13 @@ declare namespace cpFile {
@default true
*/
readonly overwrite?: boolean;

/**
Perform a fast copy-on-write clone (reflink) if possible.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear from the docs here what force does.

Maybe also mention some use-cases for needing to set it to false or 'force' as I cannot think of anything reason to not have this be just true all the time.

Copy link
Author

@Artoria2e5 Artoria2e5 Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is analogous to gnu cp's --reflink=auto vs --reflink. From a usability standpoint, it is usually preferable to have a fallback (hence true), but when someone comes onto a new platform it's nice to have a test for whether reflink is supported (hence "force").

@default true
*/
readonly clone?: boolean | 'force';
}

interface ProgressData {
Expand Down
86 changes: 73 additions & 13 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,55 @@
/// <reference path="index.d.ts" />
'use strict';
const path = require('path');
const {constants: fsConstants} = require('fs');
const pEvent = require('p-event');
const CpFileError = require('./cp-file-error');
const fs = require('./fs');
const ProgressEmitter = require('./progress-emitter');
const {version} = process;

const defaultOptions = {
overwrite: true,
clone: true
};

/**
@param {import('fs').PathLike} source
@param {import('fs').PathLike} destination
*/
const updateStats = async (source, destination) => {
const stats = await fs.lstat(source);

return Promise.all([
fs.utimes(destination, stats.atime, stats.mtime),
fs.chmod(destination, stats.mode)
]);
};

/**
@param {string} source
@param {string} destination
@param {import('.').Options} options
@param {ProgressEmitter} progressEmitter
*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the JSDoc comments should have been added in a separate PR. Try to keep that in mind in the future. It makes it much easier to review without all the unrelated noise.

const cpFileAsync = async (source, destination, options, progressEmitter) => {
let readError;
const stat = await fs.stat(source);
progressEmitter.size = stat.size;

// Try to do a fast-path using FICLONE_FORCE. This will be very fast if at all successful.
if (options.clone) {
try {
fs.cloneFileSync(source, destination, options.overwrite ? null : fsConstants.COPYFILE_EXCL);
progressEmitter.writtenBytes = progressEmitter.size;
return updateStats(source, destination);
} catch (error) {
if (options.clone === 'force') {
throw error;
}
}
}

const readStream = await fs.createReadStream(source);
await fs.makeDir(path.dirname(destination));
const writeStream = fs.createWriteStream(destination, {flags: options.overwrite ? 'w' : 'wx'});
Expand All @@ -24,13 +63,11 @@ const cpFileAsync = async (source, destination, options, progressEmitter) => {
writeStream.end();
});

let shouldUpdateStats = false;
try {
const writePromise = pEvent(writeStream, 'close');
readStream.pipe(writeStream);
await writePromise;
progressEmitter.writtenBytes = progressEmitter.size;
shouldUpdateStats = true;
} catch (error) {
throw new CpFileError(`Cannot write to \`${destination}\`: ${error.message}`, error);
}
Expand All @@ -39,39 +76,47 @@ const cpFileAsync = async (source, destination, options, progressEmitter) => {
throw readError;
}

if (shouldUpdateStats) {
const stats = await fs.lstat(source);

return Promise.all([
fs.utimes(destination, stats.atime, stats.mtime),
fs.chmod(destination, stats.mode)
]);
}
// If we make it here, we should have no other errors. No need for a flag.
return updateStats(source, destination);
};

/**
@param {string} sourcePath
@param {string} destinationPath
@param {import('.').Options} options
@returns {Promise<void> & import('.').ProgressEmitter}
*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*/
*/

Applies everywhere.

const cpFile = (sourcePath, destinationPath, options) => {
if (!sourcePath || !destinationPath) {
// @ts-expect-error
return Promise.reject(new CpFileError('`source` and `destination` required'));
}

options = {
overwrite: true,
...defaultOptions,
...options
};

const progressEmitter = new ProgressEmitter(path.resolve(sourcePath), path.resolve(destinationPath));
const promise = cpFileAsync(sourcePath, destinationPath, options, progressEmitter);

// @ts-expect-error
promise.on = (...arguments_) => {
// @ts-expect-error
progressEmitter.on(...arguments_);
return promise;
};

// @ts-expect-error
return promise;
};

module.exports = cpFile;

/**
@param {import('fs').Stats} stat
@param {string} source
*/
const checkSourceIsFile = (stat, source) => {
if (stat.isDirectory()) {
throw Object.assign(new CpFileError(`EISDIR: illegal operation on a directory '${source}'`), {
Expand All @@ -82,21 +127,36 @@ const checkSourceIsFile = (stat, source) => {
}
};

/**
@param {string} source
@param {string} destination
@param {import('.').Options} options
*/
module.exports.sync = (source, destination, options) => {
if (!source || !destination) {
throw new CpFileError('`source` and `destination` required');
}

options = {
overwrite: true,
...defaultOptions,
...options
};

const stat = fs.statSync(source);
checkSourceIsFile(stat, source);
fs.makeDirSync(path.dirname(destination));

const flags = options.overwrite ? null : fsConstants.COPYFILE_EXCL;
let flags = 0;
if (!options.overwrite) {
flags |= fsConstants.COPYFILE_EXCL;
}

if (options.clone === true) {
flags |= fsConstants.COPYFILE_FICLONE;
} else if (options.clone === 'force') {
flags |= fsConstants.COPYFILE_FICLONE_FORCE;
}

try {
fs.copyFileSync(source, destination, flags);
} catch (error) {
Expand Down
8 changes: 6 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"node": ">=10"
},
"scripts": {
"test": "xo && nyc ava && tsd"
"test": "xo && nyc ava && tsd && tsc",
"fix": "xo --fix"
},
"files": [
"cp-file-error.js",
Expand Down Expand Up @@ -54,6 +55,9 @@
"sinon": "^9.0.0",
"tsd": "^0.11.0",
"uuid": "^7.0.2",
"xo": "^0.27.2"
"xo": "^0.27.2",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try upgrading it to the latest version. That version has built-in TS support.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version already does have the commit for TS support. And to quote a common saying, "it works on my machine"...

Could be travis CI caching an old version in the node_modules for some other dep though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but the latest XO version upgrades the TS parsers and such.

"@types/graceful-fs": "^4.1.3",
"@types/nested-error-stacks": "^2.1.0",
"typescript": "^3.9.5"
}
}
16 changes: 15 additions & 1 deletion progress-emitter.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,33 @@
/// <reference path="index.d.ts" />
'use strict';
const EventEmitter = require('events');
const { EventEmitter } = require('events');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do unrelated changes.


/** @type {WeakMap<ProgressEmitter, number>} */
const writtenBytes = new WeakMap();

/** @type {import('.').ProgressEmitter} */
class ProgressEmitter extends EventEmitter {
/**
* @param {string} sourcePath
* @param {string} destinationPath
*/
constructor(sourcePath, destinationPath) {
super();
/** @type {string} */
this._sourcePath = sourcePath;
/** @type {string} */
this._destinationPath = destinationPath;
/** @type {number} */
this.size = Infinity;
}

get writtenBytes() {
// @ts-expect-error
return writtenBytes.get(this);
}

set writtenBytes(value) {
// @ts-expect-error
writtenBytes.set(this, value);
this.emitProgress();
}
Expand All @@ -27,6 +40,7 @@ class ProgressEmitter extends EventEmitter {
destinationPath: this._destinationPath,
size,
writtenBytes,
// @ts-expect-error
percent: writtenBytes === size ? 1 : writtenBytes / size
});
}
Expand Down