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

Make constructor set custom properties #92

Merged
merged 5 commits into from
Jul 28, 2016
Merged

Make constructor set custom properties #92

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
03b98ff
Make constructor set custom properties
darsain Jun 7, 2016
85a64da
Inline builtInFields
darsain Jun 13, 2016
ab7afde
Move builtInFields to the top of the file
darsain Jun 19, 2016
1e3d2fb
Replace function bindings with self
darsain Jun 19, 2016
ec3497a
Add documentation
darsain Jul 28, 2016
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: 15 additions & 6 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ function File(file) {
this.contents = file.contents || null;

this._isVinyl = true;

// Set custom properties
Object.keys(file).forEach(function(key) {
if (this.constructor.isCustomProp(key)) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be checked to be a function before calling it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll always be a function, since extends inherits static methods as well. It'd be the same as checking if file.isBuffer() is a function. Of course it is if it inherits from Vinyl :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example:

function Foo() {}

Foo.prototype.check = function () {
    console.log(this.constructor.stat());
};

Foo.stat = function () {
    return 'foo';
};

class Bar extends Foo {}

class Baz extends Foo {
    static stat() {
        return 'baz';
    }
}

new Foo().check(); // foo
new Bar().check(); // foo
new Baz().check(); // baz

this[key] = file[key];
}
}, this);
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to avoid this binding. I know self is messier but I'm always nervous about bind performance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I'll move it to self.

}

File.prototype.isBuffer = function() {
Expand Down Expand Up @@ -82,13 +89,9 @@ File.prototype.clone = function(opt) {

// Clone our custom properties
Object.keys(this).forEach(function(key) {
// Ignore built-in fields
if (key === '_contents' || key === 'stat' ||
key === 'history' || key === 'path' ||
key === 'base' || key === 'cwd') {
return;
if (this.constructor.isCustomProp(key)) {
file[key] = opt.deep ? clone(this[key], true) : this[key];
}
file[key] = opt.deep ? clone(this[key], true) : this[key];
}, this);
return file;
};
Expand Down Expand Up @@ -141,6 +144,12 @@ File.prototype.inspect = function() {
return '<File ' + inspect.join(' ') + '>';
};

File.builtInFields = ['_contents', 'contents', 'stat', 'history', 'path', 'base', 'cwd'];
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be exposed on the constructor. Not sure of a better way to do it.

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'm open to suggestions. My main concern is for it to be configurable on objects that extend from Vinyl. With this implementation you can just do:

class Vinyl2 extends Vinyl {}
Vinyl2.builtInFields = ['foo', 'bar', ...Vinyl2.builtInFields];

And everything works as expected.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a second argument to the constructor? Not sure though

Copy link

Choose a reason for hiding this comment

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

I'll just chime in and say that exposing it as a static property on the File constructor is my preferred solution. It is a clean way for supporting other solutions that extend Vinyl as is demonstrated here. If it was added to the prototype, it would have to be stepped around during clone and such to prevent clobbering. However, adding it as an argument to the constructor itself would require a sub-class to specify those additional args on each instance. Depending on how it's done, that becomes an extra burden to either the devs or the consumers. (not to mention having tons of duplicate arrays floating around in memory)

Copy link
Contributor Author

@darsain darsain Jun 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It can't be 2nd argument to the constructor, as that would imply it's upon user to decide what is a built in property, while this decision is 100% decided by the current virtual object implementation.

The alternative can be just a file scoped variable not exposed to the user:

var builtInFields = ['_contents', 'contents', 'stat', 'history', 'path', 'base', 'cwd'];

File.isCustomProp = function(key) {
  return builtInFields.indexOf(key) === -1;
};

And anyone that extends Vinyl will just extend the method as needed:

class Vinyl2 extends Vinyl {
  static isCustomProp(key) {
    return super.isCustomProp(key) && ['my', 'own', 'props'].indexOf(key) === -1;
  }
}

I'd actually even prefer this as this way we won't have to be juggling configs on constructor properties.


File.isCustomProp = function(key) {
return this.builtInFields.indexOf(key) === -1;
};

File.isVinyl = function(file) {
return (file && file._isVinyl === true) || false;
};
Expand Down
7 changes: 7 additions & 0 deletions test/File.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ describe('File', function() {
file.contents.should.equal(val);
done();
});

it('should set custom properties', function(done) {
var sourceMap = {};
var file = new File({ sourceMap: sourceMap });
file.sourceMap.should.equal(sourceMap);
done();
});
});

describe('isBuffer()', function() {
Expand Down