sindresorhus / gulp-rev

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

Question about merge support for two tasks #100

Closed fchristant closed 7 years ago

fchristant commented 9 years ago

I have seen the following comment...

https://github.com/sindresorhus/gulp-rev/pull/77#issuecomment-68076646

...yet am still confused about how to realize my merge situation, or whether it is supported. I have two individual gulp tasks, yet I want both to output to the same manifest file. Here's my code:

gulp.task('styles', function() {
    return gulp.src('css/*.css')
  .pipe(minifycss())
  .pipe(gzip({ append: false }))
  .pipe(rev())
  .pipe(gulp.dest('assets'))
  .pipe(rev.manifest('manifest.json',{
            base: './',
            merge: true // merge with the existing manifest (if one exists) 
        }))
  .pipe(gulp.dest('system/application/config/'));
});

gulp.task('scripts', function() {
    return gulp.src('js/*.js')
  .pipe(uglify())
  .pipe(gzip({ append: false }))
  .pipe(rev())
  .pipe(gulp.dest('assets'))
  .pipe(rev.manifest('manifest.json',{
            base: './',
            merge: true // merge with the existing manifest (if one exists) 
        }))
  .pipe(gulp.dest('system/application/config/'));
});

So these are 2 individual tasks that are ran independently and sometimes after each other. I'd like them to output to a single, merged manifest. Note that the manifest is in a different directory from the actual revved files. Both the assets and manifest end up in the correct directory, yet the manifest does not merge, it is overwritten by either task.

Any help is welcome.

Chalkin commented 9 years ago

plus one on this! have the same issue

ifduyue commented 9 years ago

Manifest file is in json format, you can parse and merge them by using object-assign yourself, like gulp-rev did here.

tonytonyjan commented 9 years ago

:+1:

yavorski commented 9 years ago

+1. Same issue here.

aben commented 9 years ago

Fixed! waiting for merge. https://github.com/sindresorhus/gulp-rev/pull/108

or

$ npm uninstall gulp-rev
$ npm install Aben/gulp-rev
rev.manifest({
    merge: 'path/to/existing/rev-manifest.json'
}

for now.

silverPanda commented 9 years ago

the merge is not work.......

ghidinelli commented 9 years ago

+1 - abstracting out the gulp tasks for repetitive calls (e.g., to create multiple css files from a "css" parent task) makes the manifest almost always result in a single file.

aben commented 9 years ago

@silverPanda paste code to here please.

example:

gulp.task('styles', function() {
    return gulp.src('css/*.css')
    .pipe(minifycss())
    .pipe(gzip({ append: false }))
    .pipe(rev())
    .pipe(gulp.dest('assets'))
    .pipe(rev.manifest({
        merge: 'system/application/config/manifest.json'
    }))
    .pipe(gulp.dest('system/application/config/'));
});

gulp.task('scripts', function() {
    return gulp.src('js/*.js')
    .pipe(uglify())
    .pipe(gzip({ append: false }))
    .pipe(rev())
    .pipe(gulp.dest('assets'))
    .pipe(rev.manifest({
        merge: 'system/application/config/manifest.json'
    }))
    .pipe(gulp.dest('system/application/config/'));
});
ghidinelli commented 9 years ago

@Aben - I grabbed gulp-rev 6.0.1 which appears to have your fixes in it (your repo appears to be gone now too I presume after the merge?) but I'm still getting overwritten results in the rev-manifest:

app.addStyle = function(outputFilename, paths) {

    return gulp.src(paths)
                .pipe(plumber())
                .pipe(sourcemaps.init())
                .pipe(sass())
                .pipe(concat(outputFilename))
                .pipe(minifyCSS())
                .pipe(rev())
                .pipe(sourcemaps.write('.'))
                .pipe(gulp.dest('./app/assets/global/'))
                .pipe(rev.manifest({merge: './app/assets/global/rev-manifest.json'}))
                .pipe(gulp.dest('./app/assets/global/'));

};

gulp.task('styles:sub1', function() {
    return app.addStyle('sub1.css'
                    ,['./app/assets/scss/sub1.scss']);
});

gulp.task('styles:sub2', function() {
    return app.addStyle('sub2.css'
                    ,['./app/assets/scss/sub2.scss']);
});

// meta task
gulp.task('styles', ['styles:sub1', 'styles:sub2']);

Then I'm running it as gulp styles but my rev-manifest file only winds up with one of the two files in it (and it is random as to which one wins out).

Is this approach not possible?

aben commented 9 years ago

@ghidinelli I was wrong that why I deleted my fork.

In your case, you need set cwd and absolute path for rev.manifest. try this:

var path = require('path')

.pipe(rev.manifest({
    path: path.join(__dirname, '/app/assets/global/rev-manifest.json'),
    cwd: path.join(__dirname, '/app/assets/global/'),
    merge: true
}))

@silverPanda @ghidinelli try absolute path and set cwd.

ghidinelli commented 9 years ago

@Aben - thanks, I didn't realize you deleted your fork. Unfortunately your sample above still has the same end result for me: the manifest file has a random collection of files in it. This must be related to having (in my case) 6 calls to addStyle() running in parallel and stepping on each other. I appreciate your time trying to help - not sure if there is anything else to try?

aben commented 9 years ago

@ghidinelli https://github.com/sindresorhus/gulp-rev/issues/115

ghidinelli commented 9 years ago

@Aben thanks, I moved to creating a separate task to manually generate the manifest and use run-sequence to run the manifest task after I build styles/scripts. It's working well now. For others, here's the sample: https://gist.github.com/ghidinelli/e9806063ddbfa74bed7e

terminatorheart commented 8 years ago

same issue, hope be solved

meloseven commented 8 years ago

same issue,and now I grabbed gulp-rev 7.0.0

AakashGfude commented 8 years ago

Is it working with gulp-rev 7.0.0 ?

thany commented 7 years ago

Issue persists on whatever the latest version is today.

zemd commented 7 years ago

I've just set cwd and base options for each task and manifest has been merged. hope it will help for someone

.pipe($.rev.manifest({merge: true, base: path.join(__dirname, "tmp"), cwd: path.join(__dirname, "tmp")}))
thany commented 7 years ago

I've never been able to figure out what base and cmd do, but if they can't hurt existing functionality, I guess that's good if it helps some folks.

zhouzi commented 7 years ago

I am closing this issue in favour of #205 which discuss the concurrent tasks issue.