Make constructor set custom properties

gulpjs / vinyl

Virtual file format.

MIT License

1.28k stars 105 forks source link

Make constructor set custom properties #92

Closed darsain closed 8 years ago

darsain commented 8 years ago

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 commented 8 years ago

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

darsain commented 8 years ago

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 commented 8 years ago

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

phated commented 8 years ago

@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 commented 8 years ago

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

dominicbarnes commented 8 years ago

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

darsain commented 8 years ago

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 commented 8 years ago

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

darsain commented 8 years ago

Will this do?

phated commented 8 years ago

Yeah, those look good.

phated commented 8 years ago

Published this and your previous change as 1.2.0

darsain commented 8 years ago

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

phated commented 8 years ago

@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!