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

Separate out CSS/JS lint tasks from copy-assets #2863

Merged
merged 9 commits into from
Sep 27, 2022
Merged
1 change: 1 addition & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
* text=auto eol=lf
*.min.js diff=minjs
*.min.css diff=mincss
5 changes: 4 additions & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,12 @@ jobs:
if: steps.npm-cache.outputs.cache-hit != 'true'
run: npm ci

- name: Lint
run: npm run lint

- name: Build
run: |
npm run build:assets
npm run build:compile
npm run build:package
npm run build:dist

Expand Down
4 changes: 2 additions & 2 deletions docs/contributing/coding-standards/css.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ See [testing and linting](/docs/releasing/testing-and-linting.md) for more infor

## Running the lint task

You can run the linter in gulp by running `gulp scss:lint`, or use linting in [Sublime Text](https://github.com/SublimeLinter/SublimeLinter-stylelint), [Atom](https://atom.io/packages/linter-stylelint) or [other editors that support stylelint](https://stylelint.io/user-guide/integrations/editor).
You can run the linter in Gulp by running `npm run lint:scss`, or use linting in [Sublime Text](https://github.com/SublimeLinter/SublimeLinter-stylelint), [Atom](https://atom.io/packages/linter-stylelint) or [other editors that support stylelint](https://stylelint.io/user-guide/integrations/editor).

See also [testing and linting](/docs/releasing/testing-and-linting.md).

Expand Down Expand Up @@ -338,7 +338,7 @@ Good:
}
```

### `@if` statements should be written without surrounding brackets
### `@if` statements should be written without surrounding brackets

Bad:
```
Expand Down
92 changes: 44 additions & 48 deletions docs/contributing/tasks.md
Original file line number Diff line number Diff line change
@@ -1,52 +1,49 @@
# NPM and Gulp tasks
# npm and Gulp tasks

This document describes the NPM scripts that run the application, and the gulp tasks they trigger to build files, update the package, copy assets and watch for changes.
This document describes the npm scripts that run the application, and the Gulp tasks they trigger to build files, update the package, copy assets and watch for changes.

To run the application without any tasks being triggered, see [Express app only](#express-app-only).

## NPM script aliases
## npm script aliases

NPM scripts are defined in `package.json`. These trigger a number of gulp tasks.
npm scripts are defined in `package.json`. These trigger a number of Gulp tasks.

**`npm run start` will trigger `gulp dev` that:**
- cleans the `public` folder
- compiles component nunjucks files to `public`
- copies icons to `public`
- compile sass files, add vendor prefixes and copy to `public`
- starts up the Express server and app
- starts up `gulp watch` task to watch for changes
**`npm run start` will trigger `gulp dev` that will:**
- clean the `./public` folder
- compile JavaScript and Sass, including Sass documentation (`gulp compile`)
- compile again when `.scss` and `.mjs` files change (`gulp watch`)
- start up Express, restarting when `.js`, `.mjs`, and `.json` files change

**`npm run test` will do the following:**
- compile components to HTML
- run JS tests
- run CSS lint checker
- run accessibility tests on HTML files
- run tests on the review application
- run Nunjucks macros tests
- run JavaScript tests on the review application
- run accessibility and HTML validation tests

**`npm run heroku` runs on Heroku build/PR and it:**
- compiles components' HTML
- compiles CSS & JS
- starts up Express
**`npm run heroku` runs on Heroku build/PR and it will:**
- run `npm run build:compile`
- start up Express

**`npm run build:assets` will do the following:**
- compiles CSS & JS
- compiles Sass documentation
**`npm run build:compile` will do the following:**
- output files into `./public`, or another location via the `--destination` flag
- compile JavaScript and Sass, including Sass documentation (`gulp compile`)

**`npm run build:package` will do the following:**
- clean the `package` folder
- compile component nunjucks to HTML
- copy template, macro and component.njk files for each component
- copy Sass files, add vendor prefixes and replace path to be node_modules consumption compliant
- output files into `./package`, or another location via the `--destination` flag
- clean the `./package` folder
- copy Sass files, applying Autoprefixer via PostCSS
- copy Nunjucks component template/macro files, including JSON configs
- copy JavaScript ESM source files
- compile JavaScript ESM to CommonJS (`gulp js:compile`)
- runs `npm run test:build:package` (which will test the output is correct)

**`npm run build:dist` will do the following:**
- clean the `dist` folder
- copy JS
- copy icons
- copy SASS and add vendor prefixes
- compile component nujucks files to HTML
- take version from 'all/package.json' and append it to compiled & minified JS and CSS files
- runs `npm run test:dist:package` (which will test the output is correct)
- output files into `./dist`, or another location via the `--destination` flag
- clean the `./dist` folder
- compile JavaScript and Sass, including Sass documentation (`gulp compile`)
- copy fonts and images (`gulp copy:assets`)
- append version number from `package/package.json` to compiled JavaScript and CSS files
- runs `npm run test:build:dist` (which will test the output is correct)


## Gulp tasks

Expand All @@ -57,32 +54,31 @@ Gulp tasks are defined in `gulpfile.js` and .`/tasks/gulp/` folder.
This task will:
- list out all available tasks

**`gulp test`**

This task will:
- Run scss:lint

**`gulp watch`**

This task will:
- watch for changes in .mjs, .scss and .njk files and run below tasks.
- run `gulp styles` when `.scss` files change
- run `gulp scripts` when `.mjs` files change

**`gulp styles`**

This task will:
Copy link
Contributor

@domoscargin domoscargin Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This task performs:"?

Or add a verb to the beginning of each bullet point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added verbs to all bullets, thanks 😊

- run scss lint task (`gulp scss:lint`)
- sass compilation (`gulp scss:compile`) to a destination folder that can be specified via a --destination flag
- check Sass code quality via Stylelint (`npm run lint:scss`)
- compile Sass to CSS (`gulp scss:compile`) into `./public`, or another location via the `--destination` flag

**`gulp scripts`**

This task will:
- concatenate and uglify javascript (`gulp js:compile`) to a destination folder that can be specified via a --destination flag
This task will:
- check JavaScript code quality via ESLint (`npm run lint:js`) (using JavaScript Standard Style)
- compile JavaScript ESM to CommonJS (`gulp js:compile`) into `./public`, or another location via the `--destination` flag

**`gulp compile:components`**
**`gulp compile`**

This task will:
- compile all `src/govuk/components/componentName/componentName.njk` files to HTML files
This task will:
- run sub tasks from `gulp styles` without ESLint code quality checks
- run sub tasks from `gulp scripts` without StyleLint code quality checks
- compile Sass documentation into `./sassdoc`

## Express app only

To simply run the Express app without gulp tasks being triggered, simply run `node app/start.js`.
To start the Express app without Gulp tasks being triggered, run `node app/start.js`.
98 changes: 53 additions & 45 deletions gulpfile.js
Original file line number Diff line number Diff line change
@@ -1,92 +1,100 @@
const paths = require('./config/paths.js')
const gulp = require('gulp')
const taskListing = require('gulp-task-listing')
const configPaths = require('./config/paths.js')
const taskArguments = require('./tasks/task-arguments')

// Gulp sub-tasks
require('./tasks/gulp/compile-assets.js')
require('./tasks/gulp/lint.js')
require('./tasks/gulp/watch.js')
// new tasks
require('./tasks/gulp/copy-to-destination.js')
require('./tasks/gulp/watch.js')

// Node tasks
const buildSassdocs = require('./tasks/sassdoc.js')
const runNodemon = require('./tasks/nodemon.js')
const updateDistAssetsVersion = require('./tasks/asset-version.js')
const { buildSassdocs } = require('./tasks/sassdoc.js')
const { runNodemon } = require('./tasks/nodemon.js')
const { updateDistAssetsVersion } = require('./tasks/asset-version.js')
const { cleanDist, cleanPackage, cleanPublic } = require('./tasks/clean.js')
const { npmScriptTask } = require('./tasks/run.js')

// Umbrella scripts tasks for preview ---
// Runs js lint and compilation
// --------------------------------------
/**
* Umbrella scripts tasks (for watch)
* Runs JavaScript code quality checks and compilation
*/
gulp.task('scripts', gulp.series(
'js:lint',
npmScriptTask('lint:js', ['--silent']),
'js:compile'
))

// Umbrella styles tasks for preview ----
// Runs scss lint and compilation
// --------------------------------------
/**
* Umbrella styles tasks (for watch)
* Runs Sass code quality checks and compilation
*/
gulp.task('styles', gulp.series(
'scss:lint',
npmScriptTask('lint:scss', ['--silent']),
'scss:compile'
))

// Copy assets task ----------------------
// Copies assets to taskArguments.destination (public)
// --------------------------------------
/**
* Copy assets task
* Copies assets to taskArguments.destination (public)
*/
gulp.task('copy:assets', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're referring to two things as "assets" in these tasks:

  • The static assets in src/assets
  • The compiled JS and CSS files

Maybe we should rename this to copy:static-assets or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I noticed this too!

Hope the change I went with clears things up?

Rename gulp copy-assetsgulp compile (compiles CSS, Sass, Sassdoc)
Rename gulp copy-filesgulp copy:files (compiles selected files into --destination)

Gulp also looks for .displayName properties (see Task metadata) on plain Node.js functions so I've added these so everything makes sense from the command line:

clean:dist
clean:package
clean:public

Oh and as a bonus, it'll now output the actual npm script name too (not our task wrappers)

[12:27:26] Using gulpfile ~/path/to/project/govuk-frontend/gulpfile.js
[12:27:26] Starting 'styles'...
[12:27:26] Starting 'npm run lint:scss --silent'...
[12:27:29] Finished 'npm run lint:scss --silent' after 3.68 s
[12:27:29] Starting 'scss:compile'...
[12:27:30] Finished 'scss:compile' after 409 ms
[12:27:30] Finished 'styles' after 4.09 s

return gulp.src(paths.src + 'assets/**/*')
return gulp.src(configPaths.src + 'assets/**/*')
.pipe(gulp.dest(taskArguments.destination + '/assets/'))
})

// Copy assets task for local & heroku --
// Copies files to
// taskArguments.destination (public)
// --------------------------------------
gulp.task('copy-assets', gulp.series(
'styles',
'scripts'
/**
* Compile task for local & heroku
* Runs JavaScript and Sass compilation, including Sass documentation
*/
gulp.task('compile', gulp.series(
'js:compile',
'scss:compile',
buildSassdocs
))

// Serve task ---------------------------
// Restarts node app when there is changed
// affecting js, css or njk files
// --------------------------------------
/**
* Serve task
* Restarts Node.js app when there are changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we reflect that Sass changes cause a rebuild (via the watch task)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added more detail to the watch task for .scss and .mjs files

This one is serve (via Nodemon) so will only restart for Node.js related changes

* affecting .js, .mjs and .json files
*/
gulp.task('serve', gulp.parallel(
'watch',
runNodemon
))

// Dev task -----------------------------
// Runs a sequence of task on start
// --------------------------------------
/**
* Dev task
* Runs a sequence of tasks on start
*/
gulp.task('dev', gulp.series(
cleanPublic,
'copy-assets',
buildSassdocs,
'compile',
'serve'
))

// Build package task -----------------
// Prepare package folder for publishing
// -------------------------------------
/**
* Build package task
* Prepare package folder for publishing
*/
gulp.task('build:package', gulp.series(
cleanPackage,
'copy-files',
'copy:files',
'js:compile'
))

/**
* Build dist task
* Prepare dist folder for release
*/
gulp.task('build:dist', gulp.series(
cleanDist,
'copy-assets',
'compile',
'copy:assets',
updateDistAssetsVersion
))

gulp.task('sassdoc', buildSassdocs)

// Default task -------------------------
// Lists out available tasks.
// --------------------------------------
/**
* Default task
* Lists out available tasks
*/
gulp.task('default', taskListing)
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@
"scripts": {
"postinstall": "npm ls --depth=0",
"start": "gulp dev",
"heroku": "npm run build:assets && node app/start.js",
"heroku": "npm run build:compile && node app/start.js",
"build-release": "./bin/build-release.sh",
"publish-release": "./bin/publish-release.sh",
"pre-release": "./bin/pre-release.sh",
"build:assets": "gulp copy-assets && gulp sassdoc",
"build:compile": "gulp compile",
"build:package": "gulp build:package --destination 'package' && npm run test:build:package",
"build:dist": "gulp build:dist --destination 'dist' && npm run test:build:dist",
"pretest": "npm run build:assets",
"pretest": "npm run build:compile",
"test": "jest --color --ignoreProjects \"Gulp tasks\" --maxWorkers=50%",
"test:build:package": "npx jest --selectProjects \"Gulp tasks\" --testMatch \"**/*build-package*\"",
"test:build:dist": "npx jest --selectProjects \"Gulp tasks\" --testMatch \"**/*build-dist*\"",
Expand Down
6 changes: 5 additions & 1 deletion tasks/asset-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,8 @@ async function updateDistAssetsVersion () {
return Promise.all(assetTasks)
}

module.exports = updateDistAssetsVersion
updateDistAssetsVersion.displayName = 'update-dist-assets-version'

module.exports = {
updateDistAssetsVersion
}
20 changes: 12 additions & 8 deletions tasks/clean.js
Original file line number Diff line number Diff line change
@@ -1,28 +1,32 @@
const paths = require('../config/paths.js')
const del = require('del')
const configPaths = require('../config/paths.js')

function cleanDist () {
return del([
`${paths.dist}**/*`
`${configPaths.dist}**/*`
])
}

function cleanPackage () {
return del([
`${paths.package}**`,
`!${paths.package}`,
`!${paths.package}package.json`,
`!${paths.package}govuk-prototype-kit.config.json`,
`!${paths.package}README.md`
`${configPaths.package}**`,
`!${configPaths.package}`,
`!${configPaths.package}package.json`,
`!${configPaths.package}govuk-prototype-kit.config.json`,
`!${configPaths.package}README.md`
])
}

function cleanPublic () {
return del([
`${paths.public}**/*`
`${configPaths.public}**/*`
])
}

cleanDist.displayName = 'clean:dist'
cleanPackage.displayName = 'clean:package'
cleanPublic.displayName = 'clean:public'

module.exports = {
cleanDist,
cleanPackage,
Expand Down
2 changes: 1 addition & 1 deletion tasks/gulp/copy-to-destination.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const rename = require('gulp-rename')
const configPaths = require('../../config/paths.js')
const taskArguments = require('../task-arguments')

gulp.task('copy-files', () => {
gulp.task('copy:files', () => {
return merge(
/**
* Copy files to destination with './govuk-esm' suffix
Expand Down
Loading