Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option base do nothing #150

Open
smoliakov opened this issue Feb 2, 2016 · 2 comments
Open

Option base do nothing #150

smoliakov opened this issue Feb 2, 2016 · 2 comments

Comments

@smoliakov
Copy link

Whether you define base option in manifest or not the result is the same.
Probably its better to use var revisionedFile = relPath(opts.base || file.base, file.path); instead.

smoliakov added a commit to smoliakov/gulp-rev that referenced this issue Feb 2, 2016
@matikun86
Copy link

Is there any feedback on this issue? As I think I'm having the same problem on v 7.0.0.

@ruanyl
Copy link

ruanyl commented Mar 11, 2016

Just a bit of my personal opinion about this issue.
Supply base to rev.manifest() maybe originally used for manifest() to find manifest.json when merge: true

Some investigate of the reason:
https://github.com/gulpjs/vinyl-fs/blob/4c0288f3471b4e9dbc716fb10362052377ac320b/lib%2FprepareWrite.js#L45

if you take a look at the implementation of gulp.dest(), the base you provided for rev.manifest() actually doesn't effects the output path directly. what effects the output path is file.relative which is based on file.base and file.path

for example if you have:

.pipe(rev.manifest({
  base: 'public/build'
}))
.pipe(gulp.dest('path/to'))

by default the path of manifest file is rev-manifest.json. The base here is public/build, so now the relative will be ../../rev-manifest.json

And the final path to rev-manifest.json is: path.resolve(process.cwd(), 'path/to', '../../rev-manifest.json'), so it is possible the path is out of your project folder...

So use base in manifest() isn't really a good idea, it is better to also provide path to manifest() also, so that you will know exactly what is the relative instead of having ../ in the path. for example:

.pipe(rev.manifest({
  base: 'public/build',
  path: 'public/build/rev-manifest.json'
}))
.pipe(gulp.dest('path/to'))

now the relative is always rev-manifest.json, and the dest path is what people normally expect: path/to.

It might be better to hidden the details of vinyl to the end users, I mean base,path or relative of vinyl. So that user of gulp plugin don't get confused and it's much straight forward for plugin user to understand.

I did a bit changes to gulp-rev here: https://github.com/ruanyl/gulp-easy-rev
It is modified to fit my personal use case. You may want to take a look at :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants