Upgrade kit to use Gulp 4 #659
Conversation
- upgrade gulp to 4.0.0 - substitute gulp-clean for del - remove reliance on run-sequence by using in-built gulp functions
- add PR alphagov#659 to changelog
There was a problem hiding this comment.
Cheers @olliewilliams-CH, this looks great.
We just need another review from the team and we can get it merged.
- group dependencies to GOV.UK standard
gulp/watch.js
Outdated
| const config = require('./config.json') | ||
|
|
||
| gulp.task('watch-sass', function () { | ||
| return gulp.watch(config.paths.assets + 'sass/**', {cwd: './'}, ['sass']) | ||
| return gulp.watch(config.paths.assets + 'sass/**', { cwd: './' }, gulp.series('sass')) |
There was a problem hiding this comment.
is series necessary if you only have one item?
There was a problem hiding this comment.
looked into this, gulp.task('sass') seems to work too, which reads better to me
There was a problem hiding this comment.
Fair point, it does seem to work fine so I agree going with that approach makes sense. Changes made and pushed.
- replace gulp.series on watch tasks with gulp.task
gulp/clean.js
Outdated
| '.port.tmp'], {read: false}) | ||
| .pipe(clean()) | ||
| gulp.task('clean', function (done) { | ||
| return del([config.paths.public + '*', |
There was a problem hiding this comment.
we've lost a slash here /* - does it make any difference?
| // gulp 4 requires dependency tasks to be defined before they are called. | ||
| // We'll keep our top-level tasks in this file so that they are defined at the end of the chain, after their dependencies. | ||
| gulp.task('generate-assets', gulp.parallel( | ||
| 'clean', |
There was a problem hiding this comment.
I think we probably want to run clean before the others, not in parallel - or there's a chance clean could clean out files generated by the other tasks. Previously this list was series (runSequence) but maybe clean should be series, then the others as parallel? Similar to line 34
There was a problem hiding this comment.
For reference here's the original file: https://github.com/alphagov/govuk-prototype-kit/blob/master/gulp/tasks.js
There was a problem hiding this comment.
I've re-added the slash to the public path, not quite sure if I did that on purpose in a flurry of activity or if it was just a typo. Either way, Del works fine with the slash to we'll leave it in.
Good spot with the clean task. It's now in series and will run before all other tasks inside generate-assets.
- fix public path for clean task - run clean task before all others
|
We should check if #89 is fixed as a result of updating gulp 4 |
|
Thanks very much @olliewilliams-CH ! Happy new year |
A new version of gulp is available which brings with it improvements in task scheduling and a reduction in dependency vulnerabilities. This work is to make the kit use the latest version of gulp.