Merge is broken on 3.0.0

[montogeek]

I just updated the appendExisting (https://github.com/sindresorhus/gulp-rev/issues/65#issuecomment-68610591) option to merge (https://github.com/sindresorhus/gulp-rev/releases/tag/v3.0.0) and now the manifest file is not being updated :(

My gulpfile.js

var publicPath = 'public/'
gulp.task('js', function() {
  return gulp.src(scriptsPath + 'main.js', { base: path.join(process.cwd(), 'public/app') } )
    .pipe(browserify({
      insertGlobals: true,
      debug: true
    }))
    // Bundle to a single file
    .pipe(concat('scripts/app.js'))
    .pipe(uglify())
    .pipe(rev())
    .pipe(gulp.dest(publicPath))
    .pipe(rev.manifest({
      base: publicPath,
      merge: true
    }))
    .pipe(gulp.dest(publicPath))
});

The existing rev-manifest.json file is

{
  "styles/main.css": "styles/main-23ea264f.min.css"
}

The app.js is correctly minified and rev'd but the manifest file is not updated.

[montogeek]

It works doing this trick

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

Has @bobthecow said here https://github.com/sindresorhus/gulp-rev/pull/77#issuecomment-68076646

[timcosta]

I am still unable to use this merge functionality.

gulp code being used

gulp.task("watch:coffee",function(){
    return watch('assets/js/**/*.coffee')
        .pipe(coffee())
        .pipe(print())
        .pipe(rev())
        .pipe(print())
        .pipe(gulp.dest("dist-rev/js"))
        .pipe(print())
        .pipe(rev.manifest('dist-rev/rev-manifest.json',{merge:true}))
        .pipe(print())
        .pipe(gulp.dest(''));
});

output from gulpfile

[11:39:14] Starting 'watch:coffee'...
[gulp] assets/js/parent/app.js
[gulp] assets/js/parent/app-56254886.js
[gulp] dist-rev/js/parent/app-56254886.js

rev-manifest.json after the watch finishes

"js/parent/app.js": "js/parent/app-4bf4df2a.js",

[timcosta]

Doing some work on trying to figure this out, and so far all I can tell is that the flush method passed to through2 is not being called.

[bobthecow]

IIRC we've had trouble with through2/flush before. How many coffee files is this running on? Can you try it with just one file?

[timcosta]

it is watching many files, however i am only saving/updating one file at a time during my testing. so rev is only seeing one file come through the stream

[14:30:41] Starting 'watch:coffee'...
[gulp] assets/js/parent/app.js
[gulp] assets/js/parent/app-5fa4470d.js
[gulp] dist-rev/js/parent/app-5fa4470d.js
PATH:
/Users/home/dev/dist-rev/js/parent/app-5fa4470d.js
MANIFEST
{ '/Users/home/dev/assets/js/parent/app.js': 'js/parent/app-5fa4470d.js' }
[gulp] dist-rev/js/parent/app-5fa4470d.js

[bobthecow]

You're using gulp-watch? Isn't that an endless stream (i.e. it never flushes)?

[timcosta]

Ah. That is a fair point.

<-removed, opening new issue as this is now off topic->

[greypants]

Maybe update the README with an example based on https://github.com/sindresorhus/gulp-rev/pull/77#issuecomment-68076646 ? Got it to work, but it's definitely unclear how to use it at first.

[djomlastic]

I've been playing with this for a while, and it looks like “the right one” (last example from #77) isn't right. When you don't set path to your manifest file, it uses default which is: 'rev-manifest.json'. But what that means is:

process.cwd() + '/rev-manifest.json';

What get's passed to vinyl is:

{
    path: 'rev-manifest.json',
    merge: true,
    base: process.cwd() + '/dist'
}

And when that ends up in:

path.relative(this.base, this.path); // returns  ../rev-manifest.json

...it returns '../rev-manifest.json'. So gulp.dest goes one directory back from 'dist' and writes 'rev-manifest.json' in cwd. So, if we want 'rev-manifest.json' inside 'dist', “the right one” should look like this:

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

As an example, I made a setup like this:

• cwd: /home/username/Code/playground
• base: process.cwd() + '/public/assets/js'
• gulp.dest('');

So it resolved to: '../../../rev-manifest.json', which is: '/home/rev-manifest.json', which is error (access permissions).

If this is expected behavior, maybe the docs should emphasize this, something like gulp-util does with their 'new File(obj)'. I think that people expect to see their 'rev-manifest.json' in their gulp.dest location if they don't mess with the options.

[bobthecow]

Maybe default (if you don't specify) should use base + '/rev-manifest.json'?

[djomlastic]

It's late here, so I haven't tested this, I'll try tomorrow. I think the real problem is that rev.manifest() doesn't know where gulp.dest() will save the manifest file. Or to put it this way, the problem also happens when not using merge, it just becomes obvious when trying to merge. So rev.manifest() searches for manifest file inside cwd, it doesn't find one, so it makes a new one and gulp.dest('dist') saves it to 'cwd/dist'. It's overwritten each time, but it's not an issue if you're not merging. If that is the case, there should be a way to "sync" all of them (path, base and gulp.dest()), so that rev.manifest() can find old manifest file.

[djomlastic]

Yep, rev.manifest() can't predict where you're going to write the manifest file with gulp.dest(). So getManifestFile will get you a new one, and it's callback won't bother with it (is it a new one, or an existing one) unless you're merging. And that's fine.

//...
if (opts.merge && !manifestFile.isNull()) {
//...

Default path is 'rev-manifest.json', actually process.cwd() + '/rev-manifest.json', and that is the only sensible default. It just needs to be emphasized in the docs – unless you tell it where your old manifest file is, it will assume it's in your cwd.

Now, I think this is confusing. If you do:

//...
.pipe(rev.manifest('dest/rev-manifest.json'))
//...

Your base is process.cwd(). But what gulp does:

gulp.src('dest/rev-manifest.json')
//...

Your base is: process.cwd() + '/dest'. So I think that default value for base should be everything before a file name starts. Just like with gulp.src(). Then there will be no problems with gulp.dest(), only the docs should mention that you need to send the manifest file where you told rev.manifest() to look for it (if you want merging to work). So if you're not merging, everything works as before, if you're merging, you have to set the path and gulp.dest() in sync - rev.manifest() shouldn't guess where old manifest file is. That's what I would suggest.

[nfantone]

I have also been struggling with this.

So, if we want 'rev-manifest.json' inside 'dist', “the right one” should look like this:

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

This is rather mind boggling and totally not intuitive. But it does work.

[bubenshchykov]

@nfantone thanks I just managed to get it work following your hack.. pheeeew :D

[Tinysymphony]

A same problem solved with the method proposed by @djomlastic

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

[joezimjs]

I think the merge option should just have us pass a path to the original manifest, or rev.manifest should just write the file itself instead of letting gulp.dest handle it.

[lgh06]

same with above...today is 2016/7/13... "version": "7.1.0"...gulp.dest/base/the first arg of rev.manifest,,,,confuse me a lot...