Refactor with vinyl-prepare

[phated]

This is the WIP branch using vinyl-prepare. I might also try to land #139 (including sourcemap support) in this changeset (separate commits).

[erikkemperman]

Thanks for untangling this @phated! I'll try to keep things more cleanly separated in future.

About #139, on the src side, I believe every option except sourcemaps now supports function values, passing the Files as argument. Well, and passthrough isn't called per file, obviously.

I was wondering if it makes sense to also rename our read option so it no longer collides with through2?

[phated]

@erikkemperman no problem. I would typically just squash your stuff with the github UI but I wanted to document the extra features.

I need to review all the options stuff (I want to try to centralize it in some manner) before I close that issue. I think you landed a rename (to readFile) in one of your commits. However, I'm going to be removing the ability to pass options to through2 because it was a hacky solution to us not using streams properly (now fixed).

[erikkemperman]

I looked at centralizing them when I was looking at this, figuring prepare would be the place for it. But I got stuck, mostly because you preferred glob (well, file operations) stay at vinyl-fs. Perhaps a separate module used by vinyl-fs and vinyl-prepare?

Yeah, I renamed the option to readFile internally because we now need it later than we did before (per file instead of once up front) and as such it would have clashed with through2.

[phated]

@erikkemperman I've added the sourcemap(write) stuff into my first commit (since I'm no longer putting it in vinyl-prepare). Can you take a look and give me feedback? I'm going to be applying similar logic into your commit for src

[erikkemperman]

Sure, I'll take a look. One potential downside, I guess, to not having this in prepare is that the sourcemaps will be excluded in the "dry run" scenario that got all this started...

[phated]

@erikkemperman that is one of my goals. @doowb didn't need sourcemaps in his request but I was trying to over-engineer (as I do).

[erikkemperman]

@phated Fair enough, it's just that I would have expected a dry run to include them (especially when they're to be written separately).

Reading through the original issue, it also mentioned moving the mkdirp call into write-contents. Not sure why it would matter, but doing that would get rid of the save-file.js stage, which as it stands does nothing else.

And actually, didn't you consider making mkdirp a stream, at one point?

[phated]

The mkdirp feels messy in write-contents/ so I made it a separate stream step. I think it's probably better to have a mkdirp-stream utility but I don't know the best way to do it. I really need to find the proper abstraction levels before I split things up (and take on the maintenance responsibility). The original issue was about removing all fs.* calls from the preparation logic so putting sourcemaps into vinyl-prepare breaks that contract.

[erikkemperman]

Right. It'd be slightly messy in write-contents, that's true.

I don't know enough about the original use case to judge, but my understanding is they wanted to be able to reconstruct the directory structure as it would be created by dest. But perhaps it's alright if that does not include the (possibly distinct, I imagine) location of sourcemaps.

So far this looks good, but I'd need more time than I have right now to try this in an actual project.

[phated]

@erikkemperman that's actually the opposite of what they wanted. haha. Thanks for taking a look; I'm working on the src stuff now.

[doowb]

Our main goal is to be able to use vinyl-prepare to calculate the correct destination for a file without doing any of the file system parts. This would include not creating the directory structure. We want to be able to setup the file's path properties before we do something like render templates so those properties can be used inside the templates for linking between pages.

I'll take a closer look at this when I get a chance and provide some feedback (if necessary). It might be a couple of days so don't wait on me when you're ready to merge :).

[phated]

@erikkemperman I'm sticking a bunch of stuff in this PR that is related to the prepare stream changes but I'm not sure how I'm going rebase/merge them into master.

[erikkemperman]

@phated The opposite? Maybe I should have said "predict" rather than "reconstruct" but I wasn't that far off, Shirley :-)

[erikkemperman]

I think the gulp-sourcemaps dependency can now go away, right?

I'll link this into an actual project or two tonight (GMT+2) and let you know how that goes!

[phated]

@erikkemperman I think there are a bunch of deps I need to remove with these changes. I'll double check.

Doesn't seem to make sense?

[phated]

As soon as these tests finish running, I'm going to call this PR done. There are still things to do but I don't want to keep lumping them in here.