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.

Sign up for GitHub

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

Jump to bottom

feat(clone): option to not clone buffer, support custom properties #15

Closed
wants to merge 4 commits into from
Closed

feat(clone): option to not clone buffer, support custom properties #15

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
806854b
feat(clone): option to not clone buffer, support custom properties
laurelnaiad Apr 19, 2014
506b008
fix(README): document new option contents: false for clone function
laurelnaiad Apr 19, 2014
1ffedd3
fix(clone): deep-clone all properties except contents and stat
laurelnaiad Apr 19, 2014
a3bdce3
chore(clone): depend only on lodash.cloneDeep
laurelnaiad Apr 19, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,32 @@ File.prototype.isDirectory = function() {
return this.isNull() && this.stat && this.stat.isDirectory();
};

File.prototype.clone = function() {
var clonedContents = this.isBuffer() ? cloneBuffer(this.contents) : this.contents;
File.prototype.clone = function(opt) {
if (!opt) opt = {};

var clonedContents = this.isBuffer() && opt.contents !== false ?
cloneBuffer(this.contents) :
this.contents;

var clonedStat = this.stat ? cloneStats(this.stat) : null;

return new File({
var newFile = new File({
cwd: this.cwd,
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to specify these since we are cloning all properties anyways?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worked around the inbuilt properties in my loop so as to use the constructor where I could, just in case the constructor ends up doing something more interesting in the future. Subject to your other comment, I guess that all properties could be deep cloned, and then I can test whether or not to call cloneBuffer afterward based on the flag. On the deep cloning, do you have a favorite method? How about https://github.com/pvorb/node-clone?

Copy link
Member

Choose a reason for hiding this comment

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

@stu-salsbury Not sure which is more performant, but I'm sure lodash has a pretty good deep clone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lodash it is. My preference anyway. I was looking for something small and targeted, but it's not like this is running in a browser, thankfully.

Copy link
Member

Choose a reason for hiding this comment

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

@stu-salsbury You can install only certain pieces of lodash (like http://npmjs.org/lodash.clone)

base: this.base,
path: this.path,
stat: clonedStat,
contents: clonedContents
});

// pick up custom properties, if any
Object.keys(this).forEach(function(key) {
// avoid private props and prototype props
if (this.hasOwnProperty(key) && newFile[key] === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

This should do a deep clone on items where the typeof is object (like the stat object)

newFile[key] = this[key];
}
}, this);

return newFile;
};

File.prototype.pipe = function(stream, opt) {
Expand Down
48 changes: 45 additions & 3 deletions test/File.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('File', function() {
done();
});
});

describe('isBuffer()', function() {
it('should return true when the contents are a Buffer', function(done) {
var val = new Buffer("test");
Expand Down Expand Up @@ -250,6 +250,48 @@ describe('File', function() {

done();
});

it('should copy custom properties', function(done) {
var options = {
cwd: "/",
base: "/test/",
path: "/test/test.coffee",
contents: null
};
var file = new File(options);
file.customProp = 'a custom property';

var file2 = file.clone();

file2.should.not.equal(file, 'refs should be different');
file2.cwd.should.equal(file.cwd);
file2.base.should.equal(file.base);
file2.path.should.equal(file.path);
file2.customProp.should.equal(file.customProp);

done();
});

it('should allow to reference-copy a Buffer', function(done) {
var options = {
cwd: "/",
base: "/test/",
path: "/test/test.coffee",
contents: new Buffer("test")
};
var file = new File(options);

var file2 = file.clone({ contents: false });

file2.should.not.equal(file, 'refs should be different');
file2.cwd.should.equal(file.cwd);
file2.base.should.equal(file.base);
file2.path.should.equal(file.path);
file2.contents.should.equal(file.contents, 'buffer ref should be the same');
file2.contents.toString('utf8').should.equal(file.contents.toString('utf8'));

done();
});
});

describe('pipe()', function() {
Expand Down Expand Up @@ -382,7 +424,7 @@ describe('File', function() {
process.nextTick(done);
});
});

describe('inspect()', function() {
it('should return correct format when no contents and no path', function(done) {
var file = new File();
Expand Down Expand Up @@ -445,7 +487,7 @@ describe('File', function() {
done();
});
});

describe('contents get/set', function() {
it('should work with Buffer', function(done) {
var val = new Buffer("test");
Expand Down