Skip to content

Fix error server /manage-prototype/dependencies#2333

Merged
colinrotherham merged 7 commits intomainfrom
error-server-assets
Sep 14, 2023
Merged

Fix error server /manage-prototype/dependencies#2333
colinrotherham merged 7 commits intomainfrom
error-server-assets

Conversation

@colinrotherham
Copy link
Contributor

This PR fixes a few error page issues

It picks up from changes to isolate GOV.UK Frontend versions in:

Sass variables

The error page Sass $govuk-assets-path uses /manage-prototype/dependencies/govuk-frontend but we didn't handle error server responses for this route so fonts and background images were missing:

$govuk-assets-path: '/manage-prototype/dependencies/govuk-frontend/govuk/assets/';

Fonts missing Fonts missing, errors

Nunjucks assets

The error page Nunjucks assetPath uses /plugin-assets/govuk-frontend which would typically route to a plugin version of GOV.UK Frontend rather than one internal to the Prototype Kit

This was handled but with different URLs to the CSS:

<link rel="shortcut icon" sizes="16x16 32x32 48x48" href="/plugin-assets/govuk-frontend/govuk/assets/images/favicon.ico" type="image/x-icon">
<link rel="mask-icon" href="/plugin-assets/govuk-frontend/govuk/assets/images/govuk-mask-icon.svg" color="#0b0c0c">

if (pluginName === 'govuk-frontend') {
filePath = path.join(getInternalGovukFrontendDir(), ...urlParts)
} else {

Sass variables

The error page shares the manage-prototype.scss stylesheet which is hard coded to use:

$govuk-assets-path: '/manage-prototype/dependencies/govuk-frontend/govuk/assets/';

HTTP content-type headers

GOV.UK Frontend includes fonts (*.woff, *.woff2), images (*.png, *.svg) and favicons (*.ico)

These have now been added to the error server including *.mjs with the old-but-new text/javascript type

const fileExtensionsToMimeTypes = {
js: 'application/javascript',
css: 'text/css'
}

Legacy GOV.UK Frontend

The Prototype Kit currently patches versions of GOV.UK Frontend prior to v4.4.0 (where nunjucksMacros and sass fields were added to the plugin config) but the $govuk-assets-path variable was missing leaving the default as:

$govuk-assets-path: '/govuk/assets/';

@colinrotherham colinrotherham added the 🐛 Bug Something isn't working the way it should (including incorrect wording in documentation) label Sep 14, 2023
@colinrotherham
Copy link
Contributor Author

To confirm it works, this error is back on Manage prototype pages:

The kit must be running govuk-frontend@4.5.0 internally?

It's fixed on plugin pages which appear to use govuk-frontend@4.7.0

@colinrotherham colinrotherham changed the title Fix error server /manage-prototype/dependencies assets Fix error server /manage-prototype/dependencies Sep 14, 2023
@nataliecarey
Copy link
Contributor

Historically we've tested the isolation by trying scenarios where:

  • The user is running the same Frontend version of the kit
  • The user is running a different Frontend version to the kit
  • The user has uninstalled Frontend

The case that's most relevant to supporting future versions of Frontend hasn't historically been tested. The way I'm testing this today is:

  1. mkdir ../govuk-frontend
  2. (cd ../govuk-frontend; npm init -y)
  3. npm install ../govuk-frontend

That will install a completely blank govuk-frontend showing clearly where we expect certain things to exist in the user's version.

In that scenario the kit fails to start both in main and in error-server-assets.

@colinrotherham
Copy link
Contributor Author

In that scenario the kit fails to start both in main and in error-server-assets.

Happy to accept a challenge, but some of your tests need govuk-frontend installed

Is running without GOV.UK Frontend entirely more for this one?

nataliecarey
nataliecarey previously approved these changes Sep 14, 2023
GOV.UK Frontend is no longer served from `/plugin-assets` on the error page but we didn’t handle `/manage-prototype/dependencies`
Ensures GOV.UK Frontend fonts, images, favicons etc are loaded correctly
This gives internal pages their own `$govuk-assets-path` Sass variable to internal GOV.UK Frontend to align with Nunjucks

Whilst plugin Sass can continue to use `/plugin-assets`
Whilst not strictly necessary, packaged Sass files in GOV.UK Frontend are processed by PostCSS + Autoprefixer so we should reflect this when inspecting original sources in the browser
@colinrotherham
Copy link
Contributor Author

Thanks @nataliecarey

Just a reminder that you'll now see GOV.UK Frontend v4.5.0 internally due to --save-exact

"govuk-frontend": "^4.5.0",

Prototype pages using plugin assets won't be affected

Would be good to do another PR to avoid the source map 404 issue #2333 (comment)

GOV.UK Frontend is now only loaded from `/manage-prototype/dependencies` on internal pages
@@ -1,3 +1,4 @@
{%- set assetPath = '/manage-prototype/dependencies/govuk-frontend/govuk/assets' -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just taking another look. We've got a layout we're using for Manage Prototype section which I think we should be using here, that layout sets assetPath as you are here so I'd suggest we remove the explicit assetPath and extend views/manage-prototype/layout.njk instead.

You can see this on the 404 page, I must have missed the error page when I was going through the internal pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Pushed it up 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nataliecarey Going to push again as the new layout changes application.css to manage-prototype.css

There was a knownPaths entry that expected application.css

const knownPaths = {
  '/public/stylesheets/application.css': {
    type: 'text/css',
    contents: fs.readFileSync(path.join(process.cwd(), '.tmp', 'public', 'stylesheets', 'manage-prototype.css'))
  }
}

nataliecarey
nataliecarey previously approved these changes Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 Bug Something isn't working the way it should (including incorrect wording in documentation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants