All the pull requests

[bobthecow]

This includes @jaypea's #69, with a couple of changes:

• Rename the appendExisting option to merge.
• Read the existing manifest async instead of sync when merging.

This also includes @goldibex's #75, but merged into changes from #69 (plus a few minor code style changes).

Thoughts? /cc @sindresorhus @jaypea @goldibex

[sindresorhus]

Very nice work @bobthecow @jaypea @goldibex!

Mentioning some people that needs this so they can review and make sure it solves their need: @ijy @thebuilder @davidtheclark @netm @jk- @xwk @tomhalley89 @pkuliga @didoo @c0 @codekirei @n8agrin @micahlmartin

[sindresorhus]

You can try it out with:

npm install 'bobthecow/gulp-rev#all-the-pull-requests'

[bobthecow]

All feedback addressed.

[bobthecow]

With this latest change, rev.manifest doesn't concern itself with paths or bases or anything. Those options are simply passed through to vinyl to create a manifest file. I feel like this is the ideal way to handle it, and is super flexible, but some people are going to get confused about why their manifest file gets written where it does, or why it doesn't merge...

This one won't merge, because it will be looking for a file called rev-manifest.json in cwd:

// ...
.pipe(rev())
.pipe(gulp.dest('dist'))
.pipe(rev.manifest({merge: true}))
.pipe(gulp.dest('dist'));

This one would merge (if dist/rev-manifest.json existed), but it will write the resulting manifest file to dist/dist/rev-manifest.json:

// ...
.pipe(rev())
.pipe(gulp.dest('dist'))
.pipe(rev.manifest('dist/rev-manifest.json', {merge: true}))
.pipe(gulp.dest('dist'));

This one would work perfectly, but the two different gulp.dest arguments are a bit confusing:

// ...
.pipe(rev())
.pipe(gulp.dest('dist'))
.pipe(rev.manifest('dist/rev-manifest.json', {merge: true}))
.pipe(gulp.dest(''));

This is the right one, but nobody actually knows how path, base and cwd play together in vinyl, so most people prolly won't do this on the first try:

// ...
.pipe(rev())
.pipe(gulp.dest('dist'))
.pipe(rev.manifest({base: process.cwd() + '/dist', merge: true}))
.pipe(gulp.dest('dist'));

[bobthecow]

@sindresorhus One outstanding question: Should we make the manifest backwards compatible by snagging the manifest file if it comes by in the stream? I'm happy with a BC break here, but it's your call.

[sindresorhus]

Awesome work @bobthecow :)

Should we make the manifest backwards compatible by snagging the manifest file if it comes by in the stream?

No

Sorry about the delay all. I'd like to merge this a quick as possible. Anything I can do to make it happen.

[bobthecow]

No

kk. i'll make that base change. do we want to hold off on a merge until people confirm? it doesn't look like anyone's paying attention to the issue :)

[sindresorhus]

Nah, too bad for them if it doesn't solve their use-case. They've had lots of opportunities to give feedback. We can iterate later on if needed.

[bobthecow]

base fixed in 7962cf6

[sindresorhus]

Alright. Late here, so gonna look it over one last time tomorrow morning and merge then.

[ijy]

I'll be able to give this a test tomorrow if it helps. Sorry for the delay, it's been pretty busy of late. This looks like it should solve the problem once and for all. The time and consideration put into it is certainly not unnoticed or unappreciated. Thanks guys. Will kick the tyres tomorrow.

[bobthecow]

@sindresorhus one more, from earlier in the thread:

The readme could use a bit of hierarchy help. Specifically, Integration, Sourcemaps and Works with seem to be at the wrong heading level.

[bobthecow]

@ijy Thanks! I hear you, I've been super busy myself :)

[sindresorhus]

@ijy Were you able to try it out?

[montogeek]

Merge please :)

[sindresorhus]

Landed! Thanks all :)

[bobthecow]

Thanks @sindresorhus!

[austinpray]

Sweeeeet.

[SimenB]

I wasn't able to make this work using options.base (everytime I used it, the json-file was placed in root). The I looked at the tests, and it used options.path. If I used path as well I got it to work.

Am I doing something wrong (probably), or is the documentation wrong?

EDIT: I just noticed https://github.com/sindresorhus/gulp-rev/pull/77#issuecomment-68076646. Is that still the case?

[bobthecow]

Yes, that's still the case. See the "This is the right one, but nobody actually knows how path, base and cwd play together in vinyl, so most people prolly won't do this on the first try" :)

[bobthecow]

@sindresorhus I think that might be the happiest kitty ever made.