Ensure shared task `options` are mandatory unless specified #3647

colinrotherham · 2023-05-18T14:19:27Z

This PR ensures all build task options are mandatory and type checked

I've also included some related changes:

Configure the Review app to use govuk-frontend JSDoc types again
Remove unnecessary options (like renaming *.scss to *.scss)
Adding comments to explain why files are renamed or moved

This work was split out whilst adding postcss.config.mjs and rollup.config.mjs files to each workspace

Before

This will crash as no destPath has been provided

import { scripts } from 'govuk-frontend-tasks'

// Compile GOV.UK Frontend JavaScript
scripts.compile('!(*.test).mjs', {
  destPath: '/custom/build/path'
})

After

This works by extending the build options using ...options

import { scripts } from 'govuk-frontend-tasks'

// Default build options
import { options } from './build/options.mjs'

// Compile GOV.UK Frontend JavaScript
scripts.compile('!(*.test).mjs', {
  ...options,
  destPath: '/custom/build/path'
})

romaricpascal · 2023-05-19T08:54:28Z

Tidying up looks good, cheers! Can you explain the benefits of having task.options over doing the spread ourselves?

I'm expecting that with the types not being optional anymore, we'd catch situations like the one you mentioned in the first post. Would TypeScript warn OK on your first example (for lack of the necessary options)?

import { scripts } from 'govuk-frontend-tasks'

// Compile GOV.UK Frontend JavaScript
scripts.compile('!(*.test).mjs', {
  destPath: '/custom/build/path'
})

If that's the case, I'd like to understand why we need a custom function for the merging. If it's only for freezing objects, I'm not sure it's that risky to let the objects be extended (especially as we'll be merging in the paths anyways so have a separate object per task call), but open to be convinced otherwise 😊

colinrotherham · 2023-05-19T09:22:07Z

Would you rather I used spread? Happy to change it

Options are frozen to prevent accidental edits
Options are typed with Required<TaskOptions> (not custom objects)
Task option refactors automatically update across the project

Could have alternatively used structuredClone() to prevent changes to one object affecting another?

We'd get the type checking behaviour either way which was the aim:

Argument of type '{ srcPath: string; destPath: string; hello: string; }' is not assignable to parameter of type 'Partial<AssetOptions>'

romaricpascal · 2023-05-19T09:40:50Z

As we get type checking in any case, I'd reather we have less code and just use spread to start, cheers 😊

colinrotherham · 2023-05-19T11:55:17Z

Swapped to using object spread, ready for review again

colinrotherham · 2023-05-19T12:00:23Z

package.json

@@ -45,7 +45,6 @@
  "devDependencies": {
    "@babel/core": "^7.21.8",
    "@babel/preset-env": "^7.21.5",
-    "@types/node": "^20.1.5",


We'd accidentally put this in twice

colinrotherham · 2023-05-19T12:00:53Z

packages/govuk-frontend/tasks/build/release.mjs

      srcPath: join(options.srcPath, 'govuk'),
-      destPath: options.destPath,


Don't need to repeat ourselves. The destination is already in options

colinrotherham · 2023-05-19T12:00:58Z

packages/govuk-frontend/tasks/build/release.mjs

      srcPath: join(options.srcPath, 'govuk'),
-      destPath: options.destPath,


Don't need to repeat ourselves. The destination is already in options

colinrotherham · 2023-05-19T12:03:02Z

shared/tasks/assets.mjs

@@ -84,7 +86,6 @@ export async function write (filePath, result) {
 * @typedef {object} AssetFileOptions
 * @property {(file: import('path').ParsedPath) => string} [filePath] - File path formatter
 * @property {(contents?: string) => Promise<string>} [fileContents] - File contents formatter
- * @property {string[]} [ignore] - File path patterns to ignore


We had to carefully ignore the published package.json and README.md in the past

We haven't used this option since merging:

Remove built govuk-frontend from committed files #3498

colinrotherham · 2023-05-19T12:04:50Z

shared/tasks/components.mjs

@@ -11,7 +11,7 @@ import { files } from './index.mjs'
 * Generate fixtures.json from component data
 *
 * @param {AssetEntry[0]} pattern - Path to ${componentName}.yaml
- * @param {AssetEntry[1]} options - Asset options
+ * @param {Pick<AssetEntry[1], "srcPath" | "destPath">} options - Asset options


This task already provides its own filePath and fileContents

Here we're narrowing down the types to warn when passing in options that aren't picked

colinrotherham · 2023-05-19T12:05:05Z

shared/tasks/components.mjs

@@ -44,7 +43,7 @@ export async function generateFixtures (pattern, { srcPath, destPath }) {
 * Generate macro-options.json from component data
 *
 * @param {AssetEntry[0]} pattern - Path to ${componentName}.yaml
- * @param {AssetEntry[1]} options - Asset options
+ * @param {Pick<AssetEntry[1], "srcPath" | "destPath">} options - Asset options


This task already provides its own filePath and fileContents

Here we're narrowing down the types to warn when passing in options that aren't picked

colinrotherham · 2023-05-19T12:05:12Z

shared/tasks/configs.mjs

@@ -7,7 +7,7 @@ import { files } from './index.mjs'
 * Write config to JSON
 *
 * @param {AssetEntry[0]} modulePath - File path to config
- * @param {AssetEntry[1]} options - Asset options
+ * @param {Pick<AssetEntry[1], "srcPath" | "destPath">} options - Asset options


This task already provides its own filePath and fileContents

Here we're narrowing down the types to warn when passing in options that aren't picked

colinrotherham · 2023-05-19T12:06:23Z

shared/tasks/files.mjs

@@ -11,25 +11,25 @@ import slash from 'slash'
 * Delete path globs for a given destination
 *
 * @param {string} pattern - Pattern to remove
- * @param {AssetEntry[1]} options - Asset options
+ * @param {Pick<AssetEntry[1], "destPath">} options - Asset options


The clean task only needs a destPath so we're being quite strict here

Other properties can be passed in when not TypeScript "strict": true but they'll be ignored

Config files are already JSON formatted so we can configure the filename in the shared task

The source files are already Sass so don’t need renaming

We’re not running `tsc` in strict mode so can easily break tasks by not specifying the required properties

Our main export in package.json is `"dist/govuk/all.js"` so we need to remap this to our source code for internal use only, otherwise code autocomplete doesn’t work in the review app This can be removed if we export type declarations in the package instead

romaricpascal

Cheers for updating to using spread 🙌🏻 ⛵

colinrotherham added the interoperability label May 18, 2023

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3647 May 18, 2023 14:19 Inactive

colinrotherham force-pushed the task-types branch from c57fd2f to 089f45f Compare May 18, 2023 14:25

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3647 May 18, 2023 14:26 Inactive

colinrotherham force-pushed the task-types branch from 089f45f to b2bef58 Compare May 18, 2023 14:39

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3647 May 18, 2023 14:39 Inactive

Base automatically changed from component-types to main May 18, 2023 16:42

colinrotherham force-pushed the task-types branch from b2bef58 to 1e8a152 Compare May 19, 2023 11:22

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3647 May 19, 2023 11:22 Inactive

colinrotherham force-pushed the task-types branch from 1e8a152 to 61901b7 Compare May 19, 2023 11:32

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3647 May 19, 2023 11:32 Inactive

colinrotherham commented May 19, 2023

View reviewed changes

colinrotherham added 7 commits May 19, 2023 13:38

Fix check for missing fileContents in files tasks

81f8421

Remove unused ignore option from files tasks

1518cdb

Remove unnecessary *.json file renames

4b9044a

Config files are already JSON formatted so we can configure the filename in the shared task

Remove unnecessary *.scss file renames

aebb4a5

The source files are already Sass so don’t need renaming

Ensure shared task options are mandatory unless specified

b464d62

Ensure shared task options are fully populated

aed596e

We’re not running `tsc` in strict mode so can easily break tasks by not specifying the required properties

Remove duplicate @types/node package

591311d

colinrotherham added 2 commits May 19, 2023 13:42

Ensure all task file options have comments

48f2b10

colinrotherham force-pushed the task-types branch from 61901b7 to ab15994 Compare May 19, 2023 12:42

colinrotherham changed the base branch from main to skip-lib-types May 19, 2023 12:42

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3647 May 19, 2023 12:42 Inactive

Base automatically changed from skip-lib-types to main May 19, 2023 13:36

romaricpascal approved these changes May 19, 2023

View reviewed changes

colinrotherham merged commit 2f63da6 into main May 19, 2023

colinrotherham deleted the task-types branch May 19, 2023 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure shared task `options` are mandatory unless specified #3647

Ensure shared task `options` are mandatory unless specified #3647

colinrotherham commented May 18, 2023 •

edited

Loading

romaricpascal commented May 19, 2023

colinrotherham commented May 19, 2023 •

edited

Loading

romaricpascal commented May 19, 2023

colinrotherham commented May 19, 2023

colinrotherham May 19, 2023

colinrotherham May 19, 2023

colinrotherham May 19, 2023

colinrotherham May 19, 2023

colinrotherham May 19, 2023

colinrotherham May 19, 2023

colinrotherham May 19, 2023

colinrotherham May 19, 2023

romaricpascal left a comment

		srcPath: join(options.srcPath, 'govuk'),
		destPath: options.destPath,

Ensure shared task options are mandatory unless specified #3647

Ensure shared task options are mandatory unless specified #3647

Conversation

colinrotherham commented May 18, 2023 • edited Loading

Before

After

romaricpascal commented May 19, 2023

colinrotherham commented May 19, 2023 • edited Loading

romaricpascal commented May 19, 2023

colinrotherham commented May 19, 2023

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

romaricpascal left a comment

Choose a reason for hiding this comment

Ensure shared task `options` are mandatory unless specified #3647

Ensure shared task `options` are mandatory unless specified #3647

colinrotherham commented May 18, 2023 •

edited

Loading

colinrotherham commented May 19, 2023 •

edited

Loading