From 71b68f355f4e523b3754efe0d0dc800cb6e000db Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 16 Jan 2024 16:53:07 +0000 Subject: [PATCH 1/3] Only use pre-built fixtures when running in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using `options?.fixture?.html` provides a neat speed optimisation when running the tests on CI (where we already rendered all of the fixtures to HTML and they will not change). However, for local development, we currently only update the fixtures based on watching the YAML files. This means they are not updated when the Nunjucks template changes, so tests can run on outdated versions of templates, which is confusing and potentially misleading. We discussed additionally watching for changes to the Nunjucks templates as well, however there is still a race condition when running tests (especially as the fixture generation currently takes ~1.6-1.8s). For now, only use this optimisation on CI where we can guarantee we’re testing against an up to date build. Co-authored-by: Colin Rotherham --- shared/lib/components.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/shared/lib/components.js b/shared/lib/components.js index fc7d42b23f..fb597abd2d 100644 --- a/shared/lib/components.js +++ b/shared/lib/components.js @@ -155,8 +155,12 @@ function render(componentName, options) { const macroName = componentNameToMacroName(componentName) const macroPath = `govuk/components/${componentName}/macro.njk` - // Return built fixture or render - return options?.fixture?.html ?? renderMacro(macroName, macroPath, options) + // Use built fixtures (if they exist) on CI to optimise for speed + if (process.env.CI === 'true' && options?.fixture?.html) { + return options.fixture.html + } + + return renderMacro(macroName, macroPath, options) } /** From 75c68320b06d0dbed417fbe756c47aa0e39eb605 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Tue, 16 Jan 2024 17:11:53 +0000 Subject: [PATCH 2/3] Align review app with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Try to be consistent with the approach taken in the tests by checking for the presence of the `CI` or `HEROKU_APP` environment variables. This ensures that we only enable certain optimisations when we know we’re running against an up-to-date build (on CI or when deployed to Heroku). Co-authored-by: Colin Rotherham --- packages/govuk-frontend-review/src/app.mjs | 8 +++----- .../govuk-frontend-review/src/common/middleware/docs.mjs | 4 ++-- .../govuk-frontend-review/src/common/nunjucks/index.mjs | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/govuk-frontend-review/src/app.mjs b/packages/govuk-frontend-review/src/app.mjs index 6d48f078bc..9dc8edc3be 100644 --- a/packages/govuk-frontend-review/src/app.mjs +++ b/packages/govuk-frontend-review/src/app.mjs @@ -48,8 +48,7 @@ export default async () => { // Feature flags const flags = /** @type {FeatureFlags} */ ({ - isDeployedToHeroku: !!process.env.HEROKU_APP, - isDevelopment: !['test', 'production'].includes(process.env.NODE_ENV), + isDevelopment: !process.env.HEROKU_APP && !process.env.CI, // Check for JSDoc, SassDoc and Rollup stats hasDocsScripts: await hasPath(join(paths.app, 'dist/docs/jsdoc')), @@ -221,7 +220,7 @@ export default async () => { env, // Skip Nunjucks render from cache in development - fixture: !flags.isDevelopment ? fixture : undefined + fixture: flags.isDevelopment ? undefined : fixture }) let bodyClasses = 'app-template__body' @@ -278,8 +277,7 @@ export default async () => { /** * @typedef {object} FeatureFlags - * @property {boolean} isDeployedToHeroku - Review app using `HEROKU_APP` - * @property {boolean} isDevelopment - Review app not using `NODE_ENV` production or test + * @property {boolean} isDevelopment - Review app not in CI or on Heroku * @property {boolean} hasDocsStyles - Stylesheets documentation (SassDoc) is available * @property {boolean} hasDocsScripts - JavaScripts documentation (JSDoc) is available * @property {boolean} hasStats - Rollup stats are available diff --git a/packages/govuk-frontend-review/src/common/middleware/docs.mjs b/packages/govuk-frontend-review/src/common/middleware/docs.mjs index 12b78abc60..d4e8ea0011 100644 --- a/packages/govuk-frontend-review/src/common/middleware/docs.mjs +++ b/packages/govuk-frontend-review/src/common/middleware/docs.mjs @@ -16,10 +16,10 @@ router.get('/', (req, res) => { * Sass docs latest release (when deployed) */ router.use('/sass', ({ app }, res, next) => { - const { isDeployedToHeroku } = + const { isDevelopment } = /** @type {import('../../app.mjs').FeatureFlags} */ (app.get('flags')) - if (isDeployedToHeroku) { + if (!isDevelopment) { return res.redirect( 'https://frontend.design-system.service.gov.uk/sass-api-reference/' ) diff --git a/packages/govuk-frontend-review/src/common/nunjucks/index.mjs b/packages/govuk-frontend-review/src/common/nunjucks/index.mjs index d1a53413b8..44a15778d2 100644 --- a/packages/govuk-frontend-review/src/common/nunjucks/index.mjs +++ b/packages/govuk-frontend-review/src/common/nunjucks/index.mjs @@ -22,7 +22,7 @@ export function renderer(app) { { dev: true, // log stack traces express: app, // the Express.js review app that nunjucks should install to - noCache: !flags.isDeployedToHeroku, // use cache when deployed only + noCache: flags.isDevelopment, // use cache in development only watch: flags.isDevelopment // reload templates in development only }, { From e3651dc8ca9d2163a4eca49df8e3323373390270 Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Wed, 17 Jan 2024 13:50:33 +0000 Subject: [PATCH 3/3] Avoid duplicate checks for Heroku deploys The `render` function now has its own checks to avoid using the HTML from the fixture files, so we can consolidate them into one and avoid having two slightly different checks in different parts of the code. --- packages/govuk-frontend-review/src/app.mjs | 4 +--- shared/lib/components.js | 6 ++++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/govuk-frontend-review/src/app.mjs b/packages/govuk-frontend-review/src/app.mjs index 9dc8edc3be..c3fe9a7813 100644 --- a/packages/govuk-frontend-review/src/app.mjs +++ b/packages/govuk-frontend-review/src/app.mjs @@ -218,9 +218,7 @@ export default async () => { const componentView = render(componentName, { context: fixture.options, env, - - // Skip Nunjucks render from cache in development - fixture: flags.isDevelopment ? undefined : fixture + fixture }) let bodyClasses = 'app-template__body' diff --git a/shared/lib/components.js b/shared/lib/components.js index fb597abd2d..cb615b75f9 100644 --- a/shared/lib/components.js +++ b/shared/lib/components.js @@ -155,8 +155,10 @@ function render(componentName, options) { const macroName = componentNameToMacroName(componentName) const macroPath = `govuk/components/${componentName}/macro.njk` - // Use built fixtures (if they exist) on CI to optimise for speed - if (process.env.CI === 'true' && options?.fixture?.html) { + // On Heroku / CI we know we're running against an up-to-date build so we can + // use the generated HTML from the component JSON (where it exists) to make + // things faster + if ((process.env.HEROKU_APP || process.env.CI) && options?.fixture?.html) { return options.fixture.html }