govuk-frontend icon indicating copy to clipboard operation
govuk-frontend copied to clipboard

Supporting v13 of the Prototype Kit

Open nataliecarey opened this issue 3 years ago • 2 comments

Includes:

  • Using the new plugin interface to define component includes.

nataliecarey avatar Sep 15 '22 09:09 nataliecarey

Could someone from the Design System Team take a look at the test failure here please? I tried to fix it locally before pushing but I'm not clear on the target output and the mechanism of the test itself. It's failing because of the addition of a Javascript file, I'm not sure whether I've put the file in the right location or not.

I've tested this latest update with different versions of the prototype kit, it works against versions 11-13 (and I have no reason to believe it would fail in any version which uses extensions).

There's a few things that have changed in v13 which are supported with this change:

  1. The initAll() line has been moved out of the prototype kit
  2. Frontend now uses the standard asset path for extensions
  3. The components can be auto-loaded based on this configuration but govuk-frontend components aren't auto-loaded without this configuration

I've included a window.GOVUKPrototypeKit.majorVersion variable in the Javascript and a $govuk-prototype-kit-major-version variable in the SASS. These enable the conditional logic needed to allow users to use a new version of frontend with an older version of the kit if they're not ready to upgrade the kit yet.

If these things aren't provided by govuk-frontend or the user uses an older version of govuk-frontend without it then they'll have to:

  1. Add to their routes.js:
require('govuk-prototype-kit').requests.serveDirectory('/assets',
  'node_modules/govuk-frontend/govuk/assets')
  1. Add to their application.js:
window.GOVUKPrototypeKit.documentReady(() => {
  window.GOVUKFrontend.initAll()
})
  1. Include the {% from ... import ... %} line when using components from the design system

  2. Not be able to use global styles or new link style

The user can work around points 1-3 and we (the prototype kit team) should give them a way of working around point 4 but it would be great if we can release v13 of the kit containing a version of govuk-frontend which doesn't need setting up.

nataliecarey avatar Sep 22 '22 07:09 nataliecarey

Could someone from the Design System Team take a look at the test failure here please? I tried to fix it locally before pushing but I'm not clear on the target output and the mechanism of the test itself. It's failing because of the addition of a Javascript file, I'm not sure whether I've put the file in the right location or not.

If I run npm run build:package I can see that the _govuk-prototype-kit-init.js file is being copied to two places – package/govuk/_govuk-prototype-kit-init.js and package/govuk-esm/_govuk-prototype-kit-init.js.

The govuk-esm folder exists to allow users to import GOV.UK Frontend JavaScript as ECMAScript (ES) modules.

I don't think it makes sense to include _govuk-prototype-kit-init.js in the ESM folder as it won't be relevant for those users.

Looking at the js:copy-esm task, it currently copies *.mjs and *.js files:

https://github.com/alphagov/govuk-frontend/blob/64418c826d3596f892ae9eac6608a9f3288fefe0/tasks/gulp/copy-to-destination.js#L112-L119

However, I think we probably only want to copy *.mjs. I don't think there are any *.js files that we do want to end up in govuk-esm:

➜ find src -name '*.js' -a '!' -name '*.test.js'
src/govuk/_govuk-prototype-kit-init.js
src/.eslintrc.js

So I think we can just remove the ${configPaths.src}**/*.js glob from the js:copy-esm task?

Would be good to get a second set of eyes on this from someone on the team to check I'm not missing something…

36degrees avatar Sep 22 '22 08:09 36degrees

So I think we can just remove the ${configPaths.src}**/*.js glob from the js:copy-esm task?

@nataliecarey @36degrees Just a comment to say this was picked up in https://github.com/alphagov/govuk-frontend/pull/2876 to avoid Gulp change conflicts

colinrotherham avatar Sep 26 '22 10:09 colinrotherham

I think there are three things we need to decide on to unblock this:

  • Where in the package the new CSS and JS files live
  • Where we 'author' the new CSS and JS files
  • Whether to automatically generate the nunjucksMacros array

Where the new CSS and JS files live

For the purposes of separation, my preference would be to keep the new prototype-kit specific JS and CSS files outside of the govuk folder, either in the root or in a prototype-kit folder.

node_modules/govuk-frontend
├── govuk
│   ├── assets
│   ├── components
│   ├── core
│   └── …etc…
├── prototype-kit
│   ├── init.js
│   └── init.scss
└── govuk-prototype-kit.config.json

Where we author the new CSS and JS files

At the minute, the govuk-prototype-kit.config.json file only exists in the package folder.

So when the package is built, the clean task is configured to leave the config file in place, and the version in the package folder is not added or altered by any of our tasks.

(It's worked like that since it was added in https://github.com/alphagov/govuk-frontend/pull/1102 and I don't know how intentional this choice was)

Option 1: Config in package, CSS + JS in src

This is what we currently have. The config file only exists in the package folder, the CSS and JS are copied from src.

Feels slightly messy / inconsistent?

Option 2: Only add the new CSS and JS files to the package folder

This is consistent with what we're currently doing for the config file, but I'm not sure that's a precedent we want to follow.

We generally tell people not to edit files in the package folder. Although it's unlikely we'll need to change the file often, it feels cleaner to avoid this 'exception to the rule' if we can.

Option 3: Config, CSS and JS live in src

They'd be copied at build time, with any additional processing done as part of the build process (see the next section about dynamically generating parts of the config).

I think this is my preference at the minute.

Option 4: Separate package

We release a separate npm package for the kit which either includes or depends on govuk-frontend and includes the files needed for integration.

(I've included this for completeness, but don't think we want to do this as it introduces an extra level of dependency versioning that we just don't need at this time.)

Whether to automatically generate the nunjucksMacros array

I think this is tied to the previous question about where we author the files, as it introduces a generation / modification step as part of the build process – so it probably makes sense to have the 'source' in 'src'!

If we don't do this, we probably want to add tests to ensure that the nunjucksMacros array contains all of the expected components, to avoid a scenario where we add a new component but forget to add it to the array, which would mean it would not be loaded in the prototype kit.

However, as Nat has pointed out, if we can add tests for it then we are able to programatically know what macros should be in the array, and therefore it should be relatively easy to generate it.

36degrees avatar Sep 27 '22 08:09 36degrees

I'd go with Option 3 and (controversially) delete ./package

Feels like we should be able to pull any commit, install, and build the generated files:

./node_modules
./public
./sassdoc
./package
./dist

colinrotherham avatar Sep 27 '22 09:09 colinrotherham

Separating prototype kit specific code into its own folder sounds good to me, and I'd say that both config and JS and SCSS files should live there as well (option 3), even if we don't manipulate the config file and just copy a manually authored one across. That said, I'd be in favour of building the config file rather than authoring manually and adding a test.

romaricpascal avatar Sep 27 '22 09:09 romaricpascal

I'm not impacted by the outcome, I'm just saying this to help if I can.

  1. Removing package/ would be good, it's difficult to avoid committing package files by accident and they're generated files anyway so removing them should be quite painless.
  2. It could make sense to have a govuk-prototype-kit directory with the config file, the JS and the SCSS - everything that's specifically for the kit.
  3. Auto-generating could be pretty easy given how well ordered the components are.

Would someone like to pair on this over the next few working days?

nataliecarey avatar Sep 29 '22 16:09 nataliecarey

I'm not impacted by the outcome, I'm just saying this to help if I can.

  1. Removing package/ would be good, it's difficult to avoid committing package files by accident and they're generated files anyway so removing them should be quite painless.

This is a big change and I see no reason to tie it to this work, so I'd like to consider this out of scope. If this is something we want to explore, we should consider it as part of the other future architectural changes for GOV.UK Frontend.

  1. It could make sense to have a govuk-prototype-kit directory with the config file, the JS and the SCSS - everything that's specifically for the kit.

I think this would require a change to the Prototype Kit as I think it only looks in the top-level of each node module? If this is something you're open to I think having it all within a single folder would be good 👍🏻

  1. Auto-generating could be pretty easy given how well ordered the components are.

👍🏻

Would someone like to pair on this over the next few working days?

Pairing between the two teams sounds good, but I don't think I'm personally going to have capacity for this in the next few days – but think this is good for anyone to pick up?

I have raised an issue to track this work, added it to our sprint backlog and the milestone for the next release – @nataliecarey if you have a chance to review it and make sure it covers everything we need to do, that'd be great.

36degrees avatar Sep 29 '22 16:09 36degrees

This is a big change and I see no reason to tie it to this work

Makes sense.

I think this would require a change to the Prototype Kit as I think it only looks in the top-level of each node module?

Sorry, I wasn't clear - I meant that you could put those files all together and the build job could move the config file into place.

Pairing between the two teams sounds good, but I don't think I'm personally going to have capacity for this in the next few days – but think this is good for anyone to pick up?

Anyone is welcome to reach out to me about pairing :)

nataliecarey avatar Sep 30 '22 12:09 nataliecarey

@nataliecarey I've rebased your branch just to make it a bit easier to make the fixes on our side

colinrotherham avatar Oct 14 '22 11:10 colinrotherham

@nataliecarey @36degrees I've moved the config to its new destination: https://github.com/alphagov/govuk-frontend/pull/2851/commits/9bfd2394ce3c9df2ac38c914a10e6878630c2147

Following the chat above it seems govuk-prototype-kit was preferred versus prototype-kit?

node_modules/govuk-frontend
├── govuk
│   ├── assets
│   ├── components
│   ├── core
│   └── …etc…
├── govuk-prototype-kit
│   ├── init.js
│   └── init.scss
└── govuk-prototype-kit.config.json

@36degrees You might have wondered why the "Fix Windows" PR https://github.com/alphagov/govuk-frontend/pull/2946 is the new base for this one, just means we can grab our getDirectories() helper to build the config nunjucksMacros array automatically 🎉

Tests are failing currently as I'm just doing the build script to output govuk-prototype-kit.config.json

colinrotherham avatar Oct 26 '22 15:10 colinrotherham

Tests added https://github.com/alphagov/govuk-frontend/pull/2851/commits/8d21c7f8eedbc1cc9075085ab55c5aaa92caea23 just need to output the files

colinrotherham avatar Oct 26 '22 17:10 colinrotherham

🙌 Amazing! Thanks so much all 😁

lfdebrux avatar Nov 07 '22 09:11 lfdebrux