Build tasks: Create tasks per target without `--destination` #3372

colinrotherham · 2023-03-09T20:22:09Z

This PR removes our build target --destination flag

We didn't actually use it and there's a lot of complexity including/moving/renaming files via:

isPackage
isPublic
isDist
isDev

This helps to unblock:

Include the govuk-frontend version number in CSS and JS files #3318

Where we'll need to build the JavaScript twice to include version numbers:

1x as Rollup AMD module bundles
1x as Rollup ES module bundles

Task options

All tasks like compileJavaScripts() now accept options to show what's happening:

import { compileJavaScripts } from './tasks/compile-javascripts.mjs'

// Compile GOV.UK Frontend JavaScript (AMD modules)
compileJavaScripts('**/!(*.test).mjs', {
  srcPath: join(paths.src, 'govuk'),
  destPath: join(paths.package, 'govuk'),

  filePath (file) {
    return join(file.dir, `${file.name}.js`)
  }
}),

Tests replaced with `isDev` flag support

domoscargin

Think this looks great - to my eye this simplifies these workflows a bunch. And thank you for a really easy-to-follow set of commits - despite quite a large overall codebase change.

Coupla questions in addition to a few small separate comments:

I haven't dug into the tests - are we confident they'd detect any issues with this approach? Anything we can take the opportunity to shore up?
This is a fairly big PR. Feels like we could maybe separate out the gulp-task-listing (and maybe the browser tasks?) as they don't seem to rely on --destination?

domoscargin · 2023-03-22T17:25:54Z

tasks/compile-javascripts.mjs

 import { getListing } from '../lib/file-helper.js'
 import { componentPathToModuleName } from '../lib/helper-functions.js'

 import { writeAsset } from './compile-assets.mjs'
-import { isDist, isPackage, isPublic } from './task-arguments.mjs'


domoscargin · 2023-03-22T17:33:14Z

tasks/compile-javascripts.mjs

  })

  // Compile JavaScript ESM to CommonJS
-  const bundled = await bundle[!isPackage ? 'generate' : 'write']({
+  const bundled = await bundle[moduleDestPath.endsWith('.min.js') ? 'generate' : 'write']({


domoscargin · 2023-03-22T20:31:41Z

tasks/task-arguments.mjs

-export const isDist = target === 'dist'
-export const isDev = isPublic && tasks.includes('dev')
+// Check for development task
+export const isDev = tasks.includes('dev')


domoscargin · 2023-03-22T20:35:06Z

docs/contributing/tasks.md

@@ -34,7 +34,7 @@ npm scripts are defined in `package.json`. These trigger a number of Gulp tasks.
 **`npm run build:app` will do the following:**

 - clean the `./public` folder
- output files into `./public`, or another location via the `--destination` flag


I don't do this personally, but is it worth checking whether anybody else makes use of --destination to, say, build to a temp directory while developing?

I've asked on Slack 😊

Looking good for removal so far

Split out from this PR now, but I'm happy with destPath being configurable per destination:

Build tasks: Move destination build tasks into tasks/build #3403

I think if nobody's doing anything other than standard builds anyway, let's not bother. We can always pop it back in quick enough if folks complain.

domoscargin · 2023-03-22T20:38:05Z

docs/contributing/tasks.md

@@ -63,9 +63,9 @@ npm scripts are defined in `package.json`. These trigger a number of Gulp tasks.

 ## Gulp tasks

-Gulp tasks are defined in `gulpfile.js` and .`/tasks/gulp/` folder.
+Gulp tasks are defined in `gulpfile.mjs` and .`/tasks/` folder.


Am I missing something about why the full stop is outside of the <code> element here?

Also, can we tweak it to either "and the ./tasks/ folder" or "and ./tasks/"?

Hmm how strange, I didn't even notice. It's been in since 2017 2e64f5b#diff-06cac645496a5082a65db61580c0d7b2d3890e8d164ca7f4d1193e8a28f76a68R92

To avoid lots of rebasing, mind if we add a follow up typo PR?

@domoscargin Full stop removal included in edeb6d7

domoscargin · 2023-03-22T20:39:16Z

package.json

@@ -55,7 +55,6 @@
    "gulp": "^4.0.2",
    "gulp-cli": "^2.3.0",
    "gulp-rename": "^2.0.0",
-    "gulp-task-listing": "^1.1.1",


Split out into PR #3402

domoscargin · 2023-03-22T20:39:59Z

gulpfile.mjs

-    destPath: paths.dist
-  })
-))
+gulp.task('build:app', build.app())


So satisfying!

Split out into PR #3403

colinrotherham · 2023-03-23T10:36:58Z

Thanks for the review @domoscargin

I haven't dug into the tests - are we confident they'd detect any issues with this approach? Anything we can take the opportunity to shore up?

Tests wise we've still got the postbuild:* npm scripts for package/dist. For the review app, all the Jest behaviour and component tests (plus Percy) hit http://localhost:3000 so we'll see failures for missing styles/scripts/assets

This is a fairly big PR. Feels like we could maybe separate out the gulp-task-listing (and maybe the browser tasks?) as they don't seem to rely on --destination?

I've dropped a few commits from this PR branch and moved them to:

domoscargin

Awesome, nice to simplify things.

I've diffed this against package, dist and public folders and there are no changes in output, so I think this is ready to go.

colinrotherham added the consistency label Mar 9, 2023

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 9, 2023 20:22 Inactive

colinrotherham force-pushed the build-tasks-gulpfile branch from eb8b184 to 0d43aa4 Compare March 10, 2023 07:12

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 10, 2023 07:12 Inactive

colinrotherham force-pushed the build-tasks-gulpfile branch from 0d43aa4 to 2044ef4 Compare March 10, 2023 10:41

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 10, 2023 10:42 Inactive

colinrotherham force-pushed the build-tasks branch from 9192bbd to b8a552a Compare March 10, 2023 16:40

colinrotherham requested a review from a team as a code owner March 10, 2023 16:40

colinrotherham force-pushed the build-tasks-gulpfile branch from 2044ef4 to c79a20a Compare March 10, 2023 16:53

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 10, 2023 16:53 Inactive

colinrotherham changed the base branch from build-tasks to review-app-esm March 13, 2023 10:13

colinrotherham changed the title ~~[WIP] Build tasks: Create tasks per target without --destination~~ Build tasks: Create tasks per target without --destination Mar 13, 2023

colinrotherham force-pushed the build-tasks-gulpfile branch from c79a20a to 03875b1 Compare March 13, 2023 10:46

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 13, 2023 10:46 Inactive

colinrotherham force-pushed the build-tasks-gulpfile branch from 03875b1 to 1822d0f Compare March 13, 2023 12:23

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 13, 2023 12:24 Inactive

colinrotherham force-pushed the review-app-esm branch from 16f672f to 38031a8 Compare March 13, 2023 13:33

Base automatically changed from review-app-esm to main March 13, 2023 13:46

colinrotherham force-pushed the build-tasks-gulpfile branch from 1822d0f to a1f1160 Compare March 13, 2023 14:02

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 13, 2023 14:03 Inactive

colinrotherham changed the base branch from main to build-tasks March 13, 2023 14:03

colinrotherham force-pushed the build-tasks-gulpfile branch from a1f1160 to a04c67b Compare March 14, 2023 13:17

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 14, 2023 13:17 Inactive

colinrotherham force-pushed the build-tasks-gulpfile branch from a04c67b to 52595ea Compare March 14, 2023 13:31

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 14, 2023 13:31 Inactive

colinrotherham force-pushed the build-tasks-gulpfile branch from 52595ea to 9ff9388 Compare March 14, 2023 16:47

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 14, 2023 16:47 Inactive

colinrotherham force-pushed the build-tasks branch from 9643234 to 3a5b346 Compare March 15, 2023 09:27

colinrotherham force-pushed the build-tasks-gulpfile branch from 9ff9388 to 3615dab Compare March 15, 2023 09:28

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 15, 2023 09:28 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 22, 2023 10:47 Inactive

colinrotherham force-pushed the build-tasks-gulpfile branch from a467fe2 to b05e66c Compare March 22, 2023 11:08

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 22, 2023 11:08 Inactive

colinrotherham added 10 commits March 22, 2023 16:26

Configure build directories from gulpfile.mjs

1eb4ecb

Configure build filenames from gulpfile.mjs

a153843

Configure file copy directories from gulpfile.mjs

9dfb6dd

Configure file copy for ESM from gulpfile.mjs

f7d7091

Move prototype kit task into compile-configs.mjs

9d32a0d

Configure prototype kit paths from gulpfile.mjs

8ec678a

Move asset version task into file.mjs

9658fb2

Configure asset version paths from gulpfile.mjs

e8b9328

Configure clean paths from gulpfile.mjs

a807691

Remove --destination flag support

19a6a98

Tests replaced with `isDev` flag support

colinrotherham force-pushed the build-tasks-gulpfile branch from b05e66c to d18cc43 Compare March 22, 2023 16:33

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 22, 2023 16:34 Inactive

colinrotherham force-pushed the build-tasks-gulpfile branch from d18cc43 to 6801042 Compare March 22, 2023 16:38

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 22, 2023 16:39 Inactive

domoscargin reviewed Mar 22, 2023

View reviewed changes

colinrotherham force-pushed the build-tasks-gulpfile branch from 6801042 to f0aca84 Compare March 23, 2023 08:55

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 23, 2023 08:55 Inactive

colinrotherham mentioned this pull request Mar 23, 2023

Build tasks: Consolidate browser screenshot script as tasks #3401

Merged

colinrotherham force-pushed the build-tasks-gulpfile branch from f0aca84 to 19a6a98 Compare March 23, 2023 10:14

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3372 March 23, 2023 10:15 Inactive

colinrotherham mentioned this pull request Mar 23, 2023

Build tasks: Move destination build tasks into tasks/build #3403

Merged

domoscargin approved these changes Mar 23, 2023

View reviewed changes

colinrotherham merged commit 2347d33 into main Mar 23, 2023

colinrotherham deleted the build-tasks-gulpfile branch March 23, 2023 11:10

colinrotherham mentioned this pull request Mar 30, 2023

Refactor file copying tasks to remove Gulp #2711

Closed

6 tasks

colinrotherham mentioned this pull request May 5, 2023

Fix URL file:// path handling on Windows #3584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build tasks: Create tasks per target without `--destination` #3372

Build tasks: Create tasks per target without `--destination` #3372

colinrotherham commented Mar 9, 2023 •

edited

Loading

domoscargin left a comment

domoscargin Mar 22, 2023

domoscargin Mar 22, 2023

domoscargin Mar 22, 2023

domoscargin Mar 22, 2023

colinrotherham Mar 23, 2023

colinrotherham Mar 23, 2023

domoscargin Mar 23, 2023

domoscargin Mar 22, 2023

colinrotherham Mar 23, 2023

colinrotherham Mar 23, 2023

domoscargin Mar 22, 2023

colinrotherham Mar 23, 2023 •

edited

Loading

domoscargin Mar 22, 2023

colinrotherham Mar 23, 2023

colinrotherham commented Mar 23, 2023 •

edited

Loading

domoscargin left a comment

Build tasks: Create tasks per target without --destination #3372

Build tasks: Create tasks per target without --destination #3372

Conversation

colinrotherham commented Mar 9, 2023 • edited Loading

Task options

domoscargin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham commented Mar 23, 2023 • edited Loading

domoscargin left a comment

Choose a reason for hiding this comment

Build tasks: Create tasks per target without `--destination` #3372

Build tasks: Create tasks per target without `--destination` #3372

colinrotherham commented Mar 9, 2023 •

edited

Loading

colinrotherham Mar 23, 2023 •

edited

Loading

colinrotherham commented Mar 23, 2023 •

edited

Loading