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

Fixes #980 - Update to Gulp v4 #982

Merged
merged 2 commits into from
Jan 24, 2019
Merged

Fixes #980 - Update to Gulp v4 #982

merged 2 commits into from
Jan 24, 2019

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Jan 23, 2019

With this change, npm audit gives us a clean bill of health:

found 0 vulnerabilities
  in 22121 scanned packages

First, the test plan:

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 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.

@wincent wincent requested a review from julien January 23, 2019 15:54
@wincent
Copy link
Contributor Author

wincent commented Jan 23, 2019

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() {
Copy link
Contributor Author

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.

Copy link
Contributor

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

@wincent
Copy link
Contributor Author

wincent commented Jan 23, 2019

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.
@wincent
Copy link
Contributor Author

wincent commented Jan 23, 2019

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.

@julien julien merged commit c6027b7 into liferay:2.x-develop Jan 24, 2019
@wincent wincent deleted the gulp-v4-cutover branch January 24, 2019 09:40
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

Successfully merging this pull request may close these issues.

2 participants