-
Notifications
You must be signed in to change notification settings - Fork 282
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
Fixes #980 - Update to Gulp v4 #982
Conversation
Travis keeps rejecting #981, so I've included that commit redundantly here as well. We can merge that one if it passes first, otherwise, can just bundle it up with this one. |
return gulp | ||
.src(path.join(Constants.rootDir, 'lib', 'ckeditor', '/**')) | ||
.pipe(gulp.dest(Constants.editorDistFolder)); | ||
}); | ||
|
||
gulp.task('post-cleanup', function() { | ||
gulp.task('build:strip', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this because I needed to switch everything to "build:*" format, and didn't want something that would clash with "build:clean". Not sure if it's clear, but the meaning here is that "build:clean" cleans out everything from dist but "build:strip" just strips away a subset from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wincent that seem reasonable to me
Yay, #981 passed CI. Going to rebase this one. |
With this change, `npm audit` gives us a clean bill of health: ``` found 0 vulnerabilities in 22121 scanned packages ``` First, the test plan: - Run `npm run build:assets`; output: https://gist.github.com/wincent/0eab0f3da32939d55cf60b4a751cbfa8 - Run `npm run dev`; output: https://gist.github.com/wincent/cccea44334504d914851a33734bf58e6 - Run `npm run build`: output: https://gist.github.com/wincent/d28feaf6f3499df353ee45a543530f8a - Run `npm run start` and test demo: http://localhost:9000/ - Visually inspect "dist/" contents: https://gist.github.com/wincent/6c4f0e264428316348a7b5ad46fb1af9 Now, some explanation for the changes: - We no longer need the "run-sequence" dependency because we can achieve the same effect by combining `gulp.parallel()` and `gulp.series()`. - Gulp v4 tasks must not be referenced before being used, so we have to sort everything in dependency order; that means that a task like "build" that kicks off a bunch of other tasks ends up being declared at the bottom of the file. Likewise, the "build.js" file is the one that depends on the other tasks, so we have to move the `require` statements from the top-level "gulpfile.js" into the places where they are actually needed. - "run-sequence" had an `ignoreUndefinedTasks` option that we used to conveniently skip production tasks in non-release builds; without "run-sequence", we need another way of skipping. I found the tidiest way to do this was to update `ifRelease()` to return a dummy task that prints out why a particular step is being skipped. - Given that we now have order-sensitive cross-file dependencies, I wanted to make the relationships between tasks and files more explicit. I changed all tasks to start with the filename in which they are defined, so "copy-css" becomes "css:copy" and "copy-svg" becomes "icons:copy", for example. - I found some problems with the CSS and language tasks where the error handling was broken. Now all the error callbacks are totally consistent: we call them with an error if one is present and no arguments otherwise. I also got rid of a `this.emit('end')` call that couldn't possibly work because it was in a function that is never called in a way that would correctly set `this`. - Gulp v4 uses can use functions as tasks; if they are not named then the log output just says "<anonymous>" which is not very helpful, so I set `displayName` to overwrite the name in those places where we have dynamically generated functions and can't assign a name before hand (these are the synthetically generated "skip" tasks and also the skin-related tasks that are generated by looking at the file system). - Speaking of the skin-related ones, we had a chicken-and-egg problem there because we were looking in the "dist" folder to get the skin names; this won't work in Gulp v4 because the full task graph has to be evaluated *before* we start running. This means that if you clean the "dist" folder, the task graph will be incomplete. So, in the `getJoinTasks()` helper function, I switched things to scan the "src/" directory instead. I also had to pass the `allowEmpty: true` setting to `gulp.src()` because otherwise v4 complains about some of the skins not having a "main.css" file; for some reason the "default" skin doesn't have it but the others do. - I changed `var` to `const`/`let` along the way because it is doesn't involve moving lines around and so shouldn't harm readability of the diff too much.
Last usage was removed in 5e0cbf2.
I added a commit that removes a dead dependency ("gulp-consolidate") and Travis now likes this so I dare not rebase it. I created #985 that removes "dir-compare", also dead now. |
With this change,
npm audit
gives us a clean bill of health:First, the test plan:
npm run build:assets
; output: https://gist.github.com/wincent/0eab0f3da32939d55cf60b4a751cbfa8npm run dev
; output: https://gist.github.com/wincent/cccea44334504d914851a33734bf58e6npm run build
: output: https://gist.github.com/wincent/d28feaf6f3499df353ee45a543530f8anpm run start
and test demo: http://localhost:9000/Now, some explanation for the changes:
gulp.parallel()
andgulp.series()
.require
statements from the top-level "gulpfile.js" into the places where they are actually needed.ignoreUndefinedTasks
option that we used to conveniently skip production tasks in non-release builds; without "run-sequence", we need another way of skipping. I found the tidiest way to do this was to updateifRelease()
to return a dummy task that prints out why a particular step is being skipped.this.emit('end')
call that couldn't possibly work because it was in a function that is never called in a way that would correctly setthis
.<anonymous>
" which is not very helpful, so I setdisplayName
to overwrite the name in those places where we have dynamically generated functions and can't assign a name before hand (these are the synthetically generated "skip" tasks and also the skin-related tasks that are generated by looking at the file system).getJoinTasks()
helper function, I switched things to scan the "src/" directory instead. I also had to pass theallowEmpty: true
setting togulp.src()
because otherwise v4 complains about some of the skins not having a "main.css" file; for some reason the "default" skin doesn't have it but the others do.var
toconst
/let
along the way because it is doesn't involve moving lines around and so shouldn't harm readability of the diff too much.