Make constructor set custom properties

[darsain]

Currently, when doing this:

var file = new Vinyl({
  path: __dirname + '/index.js',
  sourceMap: {}
});

sourceMap and any other custom properties are ignored. This PR will make constructor extend this with all custom props passed in the configuration object.

It'll make creating Vinyl files a bit faster and more comfy.

I'm also moving check for custom properties from that multiline if statement to a dedicated static method File.isCustomProp(key) which relies on File.builtInFields array to check if a key is a custom property.

This is than used in #clone(), as well as constructor, and could be used in #toJSON() method from other PR.

It also allows anyone that extends from Vinyl to extend their ExtendedVinyl.builtInFields with their own props that shouldn't be cloned.

[coveralls]

Coverage remained the same at 100.0% when pulling 03b98ff on darsain:constructor-custom-props into e19855d on gulpjs:master.

[phated]

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

[darsain]

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.

[phated]

Maybe a second argument to the constructor? Not sure though

[dominicbarnes]

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)

[darsain]

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.

[darsain]

Can we move forward on this? I have path normalization PR in the works (to resolve #80), with tests rewrite so they pass on windows, but it requires .isCustomProp() to be already implemented, or I'm gonna be working over merge conflicts.

[phated]

Won't be able to look at this for awhile. Real life...

[phated]

@darsain are you going to make the changes to inline the built-in fields into the isCustomProp method? I think that is the best solution so far.

[dominicbarnes]

@darsain I really like your last solution, so I agree with @phated. :)

[dominicbarnes]

This is perfect! Would love to get this merged and released soon if possible.

[darsain]

I'd wait with the release for my next PR, which will normalize paths so they don't cause issues on windows. Even Vinyl's own tests don't pass on windows because of that.

But I need this merged before I finish it, so I'm not working/submitting over merge conflicts.

[phated]

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

[darsain]

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 :)

[darsain]

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

[phated]

@darsain just realized I forgot about this PR. It looks good but needs docs.

[darsain]

Will this do?

[phated]

Yeah, those look good.

[phated]

Published this and your previous change as 1.2.0

[darsain]

Cool. Needed this to get merged to start working on path normalization PR. Hopefully I'll get to it tomorrow :)

[phated]

@darsain awesome. I'll probably be very nitpick-y about the normalization stuff because we've had a few problems recently - see: https://github.com/gulpjs/vinyl-fs/blob/master/lib/prepare-write.js#L47-L49 which needs to be moved into vinyl and most of the path stuff needs to be normalized (e.g. directory returning properties should always end in a path.sep, if file.isDirectory() the file.path should also end in path.sep and other things like that)

keep up the great work!