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

The Prototype Kit has a hardcoded reference to the assets folder which has moved in v5 #3759

Closed
3 tasks done
Tracked by #3947
36degrees opened this issue Jun 6, 2023 · 8 comments · Fixed by alphagov/govuk-prototype-kit#2306
Closed
3 tasks done
Tracked by #3947

Comments

@36degrees
Copy link
Contributor

36degrees commented Jun 6, 2023

What

The changes to our repo structure in #3498 mean that the structure of the published package will change in v5. Most files have moved inside a top-level dist folder. For example, the favicon has moved from govuk/assets/images/favicon.ico todist/govuk/assets/images/favicon.ico.

However, the govuk-branded layout in the Prototype Kit includes a hardcoded assetPath that references the old assets location.

Work with the prototype team to support passing the asset path as part of the Prototype Kit config file.

Why

We need a way for the Prototype Kit to know where to find the assets in v5, whilst still allowing it to work with older releases of GOV.UK Frontend.

Who needs to work on this

Developers, Developers from the Prototype team

Who needs to review this

Developers, Developers from the Prototype team

Done when

  • The Prototype Kit config file in GOV.UK Frontend sets the assets path correctly
  • The integration between Frontend and the Prototype Kit has been manually tested to ensure that the assets path is correctly set
  • [If possible] Automated tests have been added that ensure the Prototype Kit sets the asset paths correctly based on the config in GOV.UK Frontend
@36degrees
Copy link
Contributor Author

There's a suggested approach from @nataliecarey where the Prototype Kit adds support for a new top-level constants option which we can hook into:

 "constants": [
    {
      "name": "govukAssetPath",
      "value": "/plugin-assets/govuk-frontend/dist/govuk/assets"
    }
  ]

There's a branch of the Prototype Kit with the proposed changes we can use for testing:

mkdir kit-integration-demo
cd kit-integration-demo
npx govuk-prototype-kit create --version=alphagov/govuk-prototype-kit#frontend-v5-compatibility
npm install alphagov/govuk-frontend#pre-release-delete-package-dist # or npm link

We'll need the Prototype team to confirm they're happy with this approach and update the kit before we merge any changes on our side.

She has also flagged that in her testing the font CSS path was wrong, which can be fixed by adding /dist to the asset path in init.scss.

@colinrotherham
Copy link
Contributor

Linking this alternative idea for require.resolve() which locates the assets path (same code) for both v4 and v5

For ES modules we also have import.meta.resolve() but it needs --experimental-import-meta-resolve in Node.js so would require us to continue providing CommonJS (or UMD) support

@36degrees 36degrees changed the title Ensure the Prototype Kit continues to works with the changes to the repo structure in v5 The Prototype Kit has a hardcoded reference to the assets folder which has moved in v5 Jul 11, 2023
@36degrees 36degrees moved this from Backlog 🗄 to Sprint Backlog 🏃🏼‍♀️ in GOV.UK Design System cycle board Jul 28, 2023
@colinrotherham colinrotherham moved this from Sprint Backlog 🏃🏼‍♀️ to In progress 📝 in GOV.UK Design System cycle board Aug 3, 2023
@colinrotherham colinrotherham self-assigned this Aug 3, 2023
@romaricpascal
Copy link
Member

@colinrotherham Sorry, I stepped a bit on this piece of work. To facilitate discussions for the Prototype Kit team, I quickly spiked the approach described by Ollie in spike-prototype-kit-constant to check that it did indeed solve the 404 when used with @nataliecarey's branch, which it does 🥳. It's just a spike branch, with preview-spike-prototype-kit-constant so very throwaway, just to validate things work.

@romaricpascal
Copy link
Member

Also a few thoughts from that quick spike:

  • it’s a bit weird to have the /plugin-assets/govuk-frontend part of the path inside govuk-frontend package . It looks like an implementation detail from the Prototype Kit (where the packages content are served from) leaking into the plugins. Is that part something that could be kept inside the Prototype Kit so that plugins only specify the part that corresponds to their package (eg. with relative paths getting prefixed by /plugin-assets/<NAME_OF_THE_PLUGIN>, leaving the possibility to specify absolute paths or full URLs if assets are on a CDN)
  • the array of object structure is a bit strange to edit for that kind of mapping, where a plain object feels more natural. Are there other keys than name and value planned for this list of constants?
  • this brought to mind the question of where these constants would be documented so users would know what to tweak. But that’s a bit of a separate topic.

@colinrotherham
Copy link
Contributor

@colinrotherham Sorry, I stepped a bit on this piece of work.

Thanks @romaricpascal, no that's great

Here's the part I've been working on, running the Prototype Kit with our local GOV.UK Frontend:

@colinrotherham
Copy link
Contributor

colinrotherham commented Aug 23, 2023

I've been working on path changes in this PR:

@romaricpascal Your comments regarding /plugin-assets/govuk-frontend were right. In the Sass for example, we'd receive this path suffix using $govuk-extensions-url-context in init.scss which the kit provides

E.g. Prototype management pages use /manage-prototype/dependencies/govuk-frontend

GOV.UK Frontend paths

The following paths are used currently in various places:

  1. Base path: node_modules/govuk-frontend
  2. Entry path: node_modules/govuk-frontend/govuk/all.js
  3. Include path: node_modules/govuk-frontend/govuk

@colinrotherham colinrotherham moved this from In progress 📝 to Needs review 🔍 in GOV.UK Design System cycle board Aug 25, 2023
@colinrotherham
Copy link
Contributor

PR ready for review

Some tests are failing because scripts aren't waiting for the GitHub preview to finish installing

@colinrotherham colinrotherham moved this from Needs review 🔍 to In progress 📝 in GOV.UK Design System cycle board Aug 30, 2023
@colinrotherham
Copy link
Contributor

Happy to say this is now closed in alphagov/govuk-prototype-kit#2306

The Prototype Kit supports govuk-frontend@4.0.0 so backporting new config fields wasn't feasible

Instead we simply join /assets to the directory of our main package export (as shown above)

const { dirname, join } = require('path')

// `govuk-frontend@4` /path/to/project/node_modules/govuk-frontend/govuk/assets
// `govuk-frontend@5` /path/to/project/node_modules/govuk-frontend/dist/govuk/assets

const assetsPath = join(dirname(require.resolve('govuk-frontend')), 'assets')

@colinrotherham colinrotherham moved this from In progress 📝 to Ready to release 🚀 in GOV.UK Design System cycle board Sep 21, 2023
@colinrotherham colinrotherham removed their assignment Sep 21, 2023
@36degrees 36degrees moved this from Ready to release 🚀 to Done 🏁 in GOV.UK Design System cycle board Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants