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

Fix error server /manage-prototype/dependencies #2333

Merged
merged 7 commits into from
Sep 14, 2023

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 [email protected] internally?

It's fixed on plugin pages which appear to use [email protected]

@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?

lib/errorServer.js Outdated Show resolved Hide resolved
lib/build.js Outdated Show resolved Hide resolved
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
@colinrotherham colinrotherham merged commit 7512209 into main Sep 14, 2023
27 checks passed
@colinrotherham colinrotherham deleted the error-server-assets branch September 14, 2023 16:12
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