sindresorhus / gulp-rev

Static asset revisioning by appending content hash to filenames: `unicorn.css` → `unicorn-d41d8cd98f.css`
MIT License
1.54k stars 217 forks source link

Merge is not working with separate tasks. #205

Closed warapitiya closed 6 years ago

warapitiya commented 7 years ago

Description

I tried Running separate gulp-rev in separate gulp tasks with merge=true property. Looks like file is NOT getting replaced.

Steps to reproduce

//TASK 1
gulp.task('less', () => {
    return gulp.src(build.gulp.source.less + '/main.less')
        .pipe(less())
        .pipe(rev())
        .pipe(gulp.dest(build.gulp.dist.css))
        .pipe(rev.manifest('manifest.json', {
            merge: true // merge with the existing manifest if one exists
        }))
        .pipe(gulp.dest('dist/manifest'));
});

//TASK 2
gulp.task('contact', () => {
    return gulp.src(build.gulp.ng.vendor)
        .pipe(concat('vendor.js'))
        .pipe(uglify(config))
        .pipe(rev())
        .pipe(gulp.dest(build.gulp.dist.src))
        .pipe(rev.manifest('manifest.json', {
            merge: true // merge with the existing manifest if one exists
        }))
        .pipe(gulp.dest('dist/manifest'));
});

gulp.task('default', ['less', 'contact']);

Possible solution for this

Idea is to if there is a file exist in the given path it should read it and add the needful.

carlotrimarchi commented 7 years ago

I'm trying to use this plugin for the first time and I immediately stumbled upon this problem. What's the point of merging if it doesn't work on different tasks?

muralikg commented 7 years ago

As a workaround I have a separate task to just add versioning for all the assets which were built individually by different tasks.

gulp.task('revision', function () {
    return gulp.src(['dist/**/*.*'], {base: 'dist'})
        .pipe(rev())
        .pipe(gulp.dest('dist'))
        .pipe(rev.manifest({merge: true}))
        .pipe(gulp.dest('dist'))
})
rkuzovlev commented 7 years ago

Try to use base option and change manifest path

.pipe(rev.manifest('dist/manifest/manifest.json', {
    merge: true, // merge with the existing manifest if one exists
    base: 'dist/manifest'
 }))
nullbio commented 7 years ago

I'm having this problem as well. The above suggestion (using base and change manifest path) does nothing for me to solve it. Without this feature working I can't really use this plugin unfortunately.

Edit: I ended up doing something similar to what @muralikg is doing (breaking the tasks down further). Here is what I have: https://github.com/nullbio/abcweb/blob/master/templates/gulpfile.js.tmpl -- this is actually adequate for my use case, although I'm sure other people would like to see merge working as expected.

reklatsmasters commented 7 years ago

@warapitiya Your main problem is that you start in parallel tasks with access to the manifest file.

gulp.task('task1', () => gulp.src(...)
    /*...some steps */
    .pipe(rev())
    .pipe(gulp.dest('/output'))
    .pipe(rev.manifest({merge: true}))  // async write
    .pipe(gulp.dest('/output'))
)

gulp.task('task2', () => gulp.src(...)
    /*...some steps */
    .pipe(rev())
    .pipe(gulp.dest('/output'))
    .pipe(rev.manifest({merge: true}))  // async write
    .pipe(gulp.dest('/output'))
)

gulp.task('default', ['task1', 'tasl2'])

Tasks task1 and task2 works in parallel and can't detect previous manifest file. Try to start your tasks in sequence.

gulp.task('task1', () => gulp.src(...)
    /*...some steps */
)

gulp.task('task2', ['task1'], () => gulp.src(...)
    /*...some steps */
)

// run task2 after task1
gulp.task('default', ['task1', 'task2'])

In this case everything works. Also, you can use module run-sequence.

zraly commented 7 years ago

@reklatsmasters It still doesn't work. I think that order should not be important. It would only order records in .json differently. But it seems, that every new task always rewrites the .json file.

atheros commented 7 years ago

there is some "writing" to file done asynchronously from the task flow.

I have 4 tasks that do rev.manifest(), the transformer logs what assets were read and written.

Using run-sequence, this is what I get:

Write [ 'css/vendor.css' ]
Read [ 'css/vendor.css' ]
Write [ 'css/vendor.css', 'js/app.js' ]
Read [ 'css/vendor.css' ]
Write [ 'css/main.css', 'css/vendor.css' ]
Read [ 'css/vendor.css' ]
Write [ 'css/vendor.css', 'js/vendor.js' ]

Notice that it seems that all reads where done before anything but the first write was finished.

If I put between those tasks a pause task that just delays the execution by 100ms, it works as expected:

Write [ 'js/vendor.js' ]
Read [ 'js/vendor.js' ]
Write [ 'css/vendor.css', 'js/vendor.js' ]
Read [ 'css/vendor.css', 'js/vendor.js' ]
Write [ 'css/main.css', 'css/vendor.css', 'js/vendor.js' ]
Read [ 'css/main.css', 'css/vendor.css', 'js/vendor.js' ]
Write [ 'css/main.css', 'css/vendor.css', 'js/app.js', 'js/vendor.js' ]

Creating a separate task to only do rev() has two drawbacks:

  1. You'll be left also with the unversioned files somewhere
  2. Sourcemaps will be either unversioned or will have invalid references in versioned files (unless you replace those with an other plugin).

Btw. the pause code:

gulp.task('pause', (callback) => { setTimeout(callback, 100)});
gulp.task('build', (callback) => {
    runSequence('vendorsjs', 'pause', 'vendorcss', 'pause', 'scss', 'pause', 'app-js', ['images', 'fonts'], callback);
//    runSequence('vendorsjs', 'vendorcss', 'scss', 'app-js', ['images', 'fonts'], callback);
});

It's a really bad solution, but what else can I do?

zhouzi commented 7 years ago

I suggested a solution a while ago, implemented in gulp-revise (TLDR: have separate .rev files). This issue keeps coming up again and again, I'll propose a fix ASAP.

atheros commented 7 years ago

It turns out if I correctly return gulp jobs from a gulp task, runSequence works as expected.

mablae commented 7 years ago

@atheros Could you give an example of this? I am facing the same problem of rev-manifest.json being overwritten be asyc tasks.

runSequence seems not to work on gulp 4.0 anymore?

nedtwigg commented 6 years ago

I've tried all the workarounds listed above, couldn't get any of them to work. Would be cool if anyone can show how the trick to make the runSequence workaround work - what does "return gulp jobs from a gulp task" mean?

svivian commented 6 years ago

Any updates on this? I'm having the same issue. It's nothing to do with async as running two tasks separately and minutes apart still overwrites the manifest instead of merging. Example gulpfile:

gulp.task('jquery', function() {
    return gulp.src('node_modules/jquery/dist/jquery.min.js')
        .pipe(rev())
        .pipe(gulp.dest('static/js'))
        .pipe(rev.manifest({merge: true}))
        .pipe(gulp.dest('static/js'));
});

gulp.task('js', function() {
    return gulp.src(['variousfiles', ...]).pipe(concat('global.js')).pipe(uglify())
        .pipe(rev())
        .pipe(gulp.dest('static/js'))
        .pipe(rev.manifest({merge: true}))
        .pipe(gulp.dest('static/js'));
});

Running gulp js and gulp jquery in any order overwrites the previous one, so I only get one line in the manifest file instead of two.

@nedtwigg My current workaround is to output separate manifests for each task then merge (in a different task) with gulp-merge-json, but this is pretty annoying with multiple separate JS files.

svivian commented 6 years ago

Did some more research and finally found the solution: https://github.com/sindresorhus/gulp-rev/issues/123#issuecomment-156155960

The key is to specify the base config option correctly as well as the output file for the manifest. Somehow I'd tried every combo except this.

So with my example above the manifest lines are changed to: .pipe(rev.manifest('static/rev-manifest.json', {base: 'static', merge: true}))

edwardsnare commented 6 years ago

@warapitiya shouldn't this issue be closed now? @svivian's answer seems to be the solution