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

Ensure shared task options are mandatory unless specified #3647

Merged
merged 9 commits into from
May 19, 2023
1 change: 0 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
"devDependencies": {
"@babel/core": "^7.21.8",
"@babel/preset-env": "^7.21.5",
"@types/node": "^20.1.5",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd accidentally put this in twice

"@typescript-eslint/eslint-plugin": "^5.59.5",
"@typescript-eslint/parser": "^5.59.5",
"concurrently": "^8.0.1",
Expand Down
7 changes: 5 additions & 2 deletions packages/govuk-frontend-review/tasks/scripts.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@ import gulp from 'gulp'
export const compile = (options) => gulp.series(
task.name('compile:js', () =>
scripts.compile('all.mjs', {
...options,

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

filePath (file) {
return join(file.dir, `${file.name}.min.js`)
// Rename with `*.min.js` extension
filePath ({ dir, name }) {
return join(dir, `${name}.min.js`)
}
})
),
Expand Down
7 changes: 5 additions & 2 deletions packages/govuk-frontend-review/tasks/styles.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@ import gulp from 'gulp'
export const compile = (options) => gulp.series(
task.name('compile:scss', () =>
styles.compile('**/[!_]*.scss', {
...options,

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

filePath (file) {
return join(file.dir, `${file.name}.min.css`)
// Rename with `*.min.css` extension
filePath ({ dir, name }) {
return join(dir, `${name}.min.css`)
}
})
),
Expand Down
7 changes: 6 additions & 1 deletion packages/govuk-frontend-review/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,10 @@
"../../shared/lib",
"../../shared/tasks"
],
"exclude": ["./dist/**", "../govuk-frontend/dist/**"]
"exclude": ["./dist/**", "../govuk-frontend/dist/**"],
"compilerOptions": {
"paths": {
"govuk-frontend": ["../govuk-frontend/src/govuk/all.mjs"]
}
}
}
4 changes: 1 addition & 3 deletions packages/govuk-frontend/tasks/build/package.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ import { fixtures, scripts, styles, templates } from '../index.mjs'
*/
export default (options) => gulp.series(
task.name('clean', () =>
files.clean('*', {
destPath: options.destPath
})
files.clean('*', options)
),

fixtures(options),
Expand Down
24 changes: 12 additions & 12 deletions packages/govuk-frontend/tasks/build/release.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ import gulp from 'gulp'
*/
export default (options) => gulp.series(
task.name('clean', () =>
files.clean('*', {
destPath: options.destPath
})
files.clean('*', options)
),

// Copy GOV.UK Frontend static assets
Expand All @@ -28,31 +26,33 @@ export default (options) => gulp.series(
// Compile GOV.UK Frontend JavaScript
task.name('compile:js', () =>
scripts.compile('all.mjs', {
...options,

srcPath: join(options.srcPath, 'govuk'),
destPath: options.destPath,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


filePath (file) {
return join(file.dir, `${file.name.replace(/^all/, pkg.name)}-${pkg.version}.min.js`)
// Rename using package name (versioned) and `*.min.js` extension
filePath ({ dir, name }) {
return join(dir, `${name.replace(/^all/, pkg.name)}-${pkg.version}.min.js`)
}
})
),

// Compile GOV.UK Frontend Sass
task.name('compile:scss', () =>
styles.compile('**/[!_]*.scss', {
...options,

srcPath: join(options.srcPath, 'govuk'),
destPath: options.destPath,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


filePath (file) {
return join(file.dir, `${file.name.replace(/^all/, pkg.name)}-${pkg.version}.min.css`)
// Rename using package name (versioned) and `*.min.css` extension
filePath ({ dir, name }) {
return join(dir, `${name.replace(/^all/, pkg.name)}-${pkg.version}.min.css`)
}
})
),

// Update GOV.UK Frontend version
task.name("file:version 'VERSION.txt'", () =>
files.version('VERSION.txt', {
destPath: options.destPath
})
files.version('VERSION.txt', options)
)
)
15 changes: 8 additions & 7 deletions packages/govuk-frontend/tasks/scripts.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ export const compile = (options) => gulp.series(
*/
task.name('compile:mjs', () =>
scripts.compile('!(*.test).mjs', {
...options,

srcPath: join(options.srcPath, 'govuk'),
destPath: join(options.destPath, 'govuk-esm')
})
Expand All @@ -24,11 +26,14 @@ export const compile = (options) => gulp.series(
*/
task.name('compile:js', () =>
scripts.compile('**/!(*.test).mjs', {
...options,

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

filePath (file) {
return join(file.dir, `${file.name}.js`)
// Rename with `*.js` extension
filePath ({ dir, name }) {
return join(dir, `${name}.js`)
}
})
),
Expand All @@ -39,11 +44,7 @@ export const compile = (options) => gulp.series(
task.name("compile:js 'govuk-prototype-kit'", () =>
configs.compile('govuk-prototype-kit.config.mjs', {
srcPath: join(options.srcPath, 'govuk-prototype-kit'),
destPath: resolve(options.destPath, '../'), // Top level (not dist) for compatibility

filePath (file) {
return join(file.dir, `${file.name}.json`)
}
destPath: resolve(options.destPath, '../') // Top level (not dist) for compatibility
})
)
)
16 changes: 6 additions & 10 deletions packages/govuk-frontend/tasks/styles.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@ export const compile = (options) => gulp.series(
*/
task.name('compile:scss', () =>
styles.compile('**/*.scss', {
srcPath: join(options.srcPath, 'govuk'),
destPath: join(options.destPath, 'govuk'),
...options,

filePath (file) {
return join(file.dir, `${file.name}.scss`)
}
srcPath: join(options.srcPath, 'govuk'),
destPath: join(options.destPath, 'govuk')
})
),

Expand All @@ -28,12 +26,10 @@ export const compile = (options) => gulp.series(
*/
task.name("compile:scss 'govuk-prototype-kit'", () =>
styles.compile('init.scss', {
srcPath: join(options.srcPath, 'govuk-prototype-kit'),
destPath: join(options.destPath, 'govuk-prototype-kit'),
...options,

filePath (file) {
return join(file.dir, `${file.name}.scss`)
}
srcPath: join(options.srcPath, 'govuk-prototype-kit'),
destPath: join(options.destPath, 'govuk-prototype-kit')
})
)
)
11 changes: 6 additions & 5 deletions shared/tasks/assets.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export async function write (filePath, result) {
writeTasks.push(files.write(base, {
destPath: dir,

// Add source code
async fileContents () {
return code
}
Expand All @@ -47,6 +48,7 @@ export async function write (filePath, result) {
writeTasks.push(files.write(`${base}.map`, {
destPath: dir,

// Add source map as JSON
async fileContents () {
return JSON.stringify(map)
}
Expand All @@ -72,10 +74,10 @@ export async function write (filePath, result) {
* Asset path options
*
* @typedef {object} AssetPathOptions
* @property {string} [basePath] - Base directory, for example `/path/to/package`
* @property {string} [srcPath] - Input directory, for example `/path/to/package/src`
* @property {string} [destPath] - Output directory, for example `/path/to/package/dist`
* @property {string} [workspace] - Workspace directory (relative), for example `package/dist`
* @property {string} basePath - Base directory, for example `/path/to/package`
* @property {string} srcPath - Input directory, for example `/path/to/package/src`
* @property {string} destPath - Output directory, for example `/path/to/package/dist`
* @property {string} workspace - Workspace directory (relative), for example `package/dist`
*/

/**
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

We haven't used this option since merging:

*/

/**
Expand Down
10 changes: 4 additions & 6 deletions shared/tasks/components.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*/
export async function generateFixtures (pattern, { srcPath, destPath }) {
const componentDataPaths = await getListing(srcPath, pattern)
Expand All @@ -22,15 +22,14 @@ export async function generateFixtures (pattern, { srcPath, destPath }) {

// Write to destination
await files.write(componentDataPath, {
srcPath,
destPath,

// Rename to fixtures.json
filePath ({ dir }) {
return join(dir, 'fixtures.json')
},

// Replace contents with JSON
// Add fixtures as JSON (formatted)
async fileContents () {
return JSON.stringify(fixture, null, 4)
}
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*/
export async function generateMacroOptions (pattern, { srcPath, destPath }) {
const componentDataPaths = await getListing(srcPath, pattern)
Expand All @@ -55,15 +54,14 @@ export async function generateMacroOptions (pattern, { srcPath, destPath }) {

// Write to destination
await files.write(componentDataPath, {
srcPath,
destPath,

// Rename to 'macro-options.json'
filePath ({ dir }) {
return join(dir, 'macro-options.json')
},

// Replace contents with JSON
// Add macro options as JSON (formatted)
async fileContents () {
return JSON.stringify(macroOption, null, 4)
}
Expand Down
9 changes: 7 additions & 2 deletions shared/tasks/configs.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

* @returns {Promise<void>}
*/
export async function compile (modulePath, options) {
Expand All @@ -17,7 +17,12 @@ export async function compile (modulePath, options) {
return files.write(modulePath, {
...options,

// Format config as JSON
// Rename with `*.json` extension
filePath ({ dir, name }) {
return join(dir, `${name}.json`)
},

// Add config as JSON (formatted)
async fileContents () {
return JSON.stringify(await configFn(), undefined, 2)
}
Expand Down
25 changes: 11 additions & 14 deletions shared/tasks/files.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*/
export async function clean (pattern, { destPath, ignore }) {
export async function clean (pattern, { destPath }) {
await deleteAsync(slash(join(destPath, pattern)), {
cwd: paths.root,
ignore
cwd: paths.root
})
}

/**
* Write `packages/govuk-frontend/package.json` version to file
*
* @param {AssetEntry[0]} assetPath - File path to asset
* @param {AssetEntry[1]} options - Asset options
* @param {Pick<AssetEntry[1], "destPath">} options - Asset options
*/
export async function version (assetPath, options) {
await write(assetPath, {
...options,

// Add package version
async fileContents () {
return pkg.version
}
Expand All @@ -40,13 +40,13 @@ export async function version (assetPath, options) {
* Write file task
*
* @param {AssetEntry[0]} assetPath - File path to asset
* @param {AssetEntry[1]} options - Asset options
* @param {Pick<AssetEntry[1], "destPath" | "filePath" | "fileContents">} options - Asset options
*/
export async function write (assetPath, { destPath, filePath, fileContents }) {
const assetDestPath = join(destPath, filePath ? filePath(parse(assetPath)) : assetPath)

if (!fileContents) {
throw new Error("Option 'fileContents' required")
if (!destPath || !fileContents) {
throw new Error("Options 'destPath' and 'fileContents' required")
}

await mkdir(dirname(assetDestPath), { recursive: true })
Expand All @@ -58,13 +58,10 @@ export async function write (assetPath, { destPath, filePath, fileContents }) {
* Copies files to destination
*
* @param {string} pattern - Minimatch pattern
* @param {AssetEntry[1]} options - Asset options
* @param {Pick<AssetEntry[1], "srcPath" | "destPath">} options - Asset options
*/
export async function copy (pattern, { srcPath, destPath, ignore = [] }) {
const srcPatterns = [slash(join(srcPath, pattern))]
.concat(ignore.map((pattern) => `!${pattern}`))

await cpy(srcPatterns, destPath, { cwd: srcPath })
export async function copy (pattern, { srcPath, destPath }) {
await cpy([slash(join(srcPath, pattern))], destPath, { cwd: srcPath })
}

/**
Expand Down
2 changes: 1 addition & 1 deletion shared/tasks/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ export * as task from './task.mjs'
/**
* Types for tasks
*
* @typedef {Required<import('./assets.mjs').AssetPathOptions>} TaskOptions
* @typedef {import('./assets.mjs').AssetPathOptions} TaskOptions
* @typedef {(options: TaskOptions) => import('gulp').TaskFunction} TaskFunction
*/
3 changes: 1 addition & 2 deletions shared/tasks/scripts.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ import { isDev } from './helpers/task-arguments.mjs'
* Compile JavaScript task
*
* @param {string} pattern - Minimatch pattern
* @param {AssetEntry[1]} [options] - Asset options
* @returns {Promise<void>}
* @param {AssetEntry[1]} options - Asset options
*/
export async function compile (pattern, options) {
const modulePaths = await getListing(options.srcPath, pattern)
Expand Down
Loading