-
Notifications
You must be signed in to change notification settings - Fork 93
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
Why specify path to HTML file? #8
Comments
Yeah, I did think about that when making the plugin, but how do you specify which files to inject in a good way then? |
Have you looked at https://github.com/zont/gulp-usemin? |
Actually I think that I think it's more inline with the "gulp-way" to use Although, I'm thinking of changing this plugin to accept a Vinyl File Stream as first parameter instead of a filename, which I think would be the most "gulpy" solution. Like this: gulp.src('./src/*.js', './src/*.css')
.pipe(inject(gulp.src('./src/index.html')))
.pipe(gulp.dest('./build')); That should be a minor fix. But maybe the other way around is more clear: gulp.src('./src/index.html')
.pipe(inject(gulp.src('./src/*.js', './src/*.css')))
.pipe(gulp.dest('./build')); That will break backwards compatibility though, but I think clarity is more important. What do you think? |
Ah. I didn't look at gulp-usemin's source. I just thought their API was a little cleaner (although it's not perfect). As a side note, it looks like they're using
This is a little better, but why do you need to read the JS and CSS files? I envision this plugin as something like a wrapper around gulp-replace: it scans your files for HTML comments and replaces the referenced resources with whatever you specify. For example, in gulp-usemin, this:
becomes
with
This is nice in that the plugin doesn't have to concern itself with where |
Actually I don't need to read the files to inject which is why I recommend you to use the But
As you can see in my last statement,
In theory yes, but because the usemin plugin reads and concatenates files this is not practical in reality. Another downside with To accomplish something similar to index.html: ...
<body>
<!-- inject:js -->
<!-- endinject -->
</body> gulpfile.js (production task): gulp.src('./src/**/*.js')
.pipe(concat('package.js'))
.pipe(gulp.dest('./build')) // Saving unminified source
.pipe(uglify())
.pipe(rename('package.min.js'))
.pipe(gulp.dest('./build')) // Saving minified source
.pipe(inject('./src/index.html', {ignorePath: '/build'})) // Injecting 'package.min.js'
.pipe(gulp.dest('./build')); // Saving index.html gulpfile.js (development task): gulp.src('./src/**/*.js', {read: false})
.pipe(inject('./src/index.html', {ignorePath: '/build'})) // Injecting 'package.min.js'
.pipe(gulp.dest('./src')); // Saving index.html at its location Which I think is much clearer than the |
I agree with all this. gulp-usemin does too many things. They shouldn't concern themselves with concatenation and minification. However, I think they recently changed to not doing any processing by default. I know the way I ended up using it, it just replaces things between comments. This is how I think gulp-inject should behave. Just replace things between specified comments.
I think there are pros and cons to this approach, but since gulp-usemin already does it, there's no sense in reimplementing what it does. So let's say gulp-inject gets the name of the file to inject from an option passed into it, e.g.
Would that work? Remember, the name of the file doesn't actually have to point to a file. It's just a string that should be injected as |
Your last example is no different than the current functionality, except it's reversed, i.e. you inject one javascript file into many html files, but I will settle with this approach, as earlier mentioned: gulp.src('./src/index.html')
.pipe(inject(gulp.src('./src/*.js', './src/*.css')))
.pipe(gulp.dest('./build')); With that solution |
I hate to ask questions in an issue, but here goes. It seems "un-gulpy" to pass in a file path and to use
fs
to read that file. Isn't the gulp way to read it from the passed in stream? Is there a reason for this?The text was updated successfully, but these errors were encountered: