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

Include the govuk-frontend version number in CSS and JS files #3318

Merged
merged 5 commits into from
Apr 3, 2023

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Feb 21, 2023

Havin' a bash at including the version of govuk-frontend being used in the compiled CSS and JS files.

Having this information available programatically will be useful in the near future, as we'll be supporting v4 and v5 simultaneously for a period of time and will benefit from being able to quickly locate the version of Frontend being used. It may also help us measure how quickly and widely v5 is adopted.

This is a little out of my wheelhouse, so I'm happy for other devs to pair and/or pick this up instead and do it better!

Closes #1734.

Changes

This PR makes the govuk-frontend version number available in CSS and JavaScript.

User-facing

In CSS, it's attached as a custom property (variable) on the :root element and can be found through the element inspector:

:root {
  --govuk-frontend-version: "4.5.0";
}

It's also included as part of the default JavaScript exports, and can be queried via the browser console:

console.log(window.GOVUKFrontend.version)
> "4.5.0"

Behind the scenes

A new JavaScript and a new Sass file have been added to the project. By default, they list the current version as "development".

src/govuk/common/govuk-frontend-version.mjs

export var version = 'development'

src/govuk/core/_govuk-frontend-version.scss

:root {
  --govuk-frontend-version: "development";
}

These files are imported into JS/Sass entry files as normal.

During the build processes, Rollup (for JS) and PostCSS (for Sass/CSS) look for these snippets of code and swaps them out for the version number listed in package/package.json.

@querkmachine querkmachine requested a review from a team February 21, 2023 16:05
@querkmachine querkmachine self-assigned this Feb 21, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3318 February 21, 2023 16:05 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3318 February 21, 2023 16:12 Inactive
@querkmachine querkmachine changed the title Include the current version of govuk-frontend in CSS and JS files Include the govuk-frontend version number in CSS and JS files Feb 21, 2023
@colinrotherham
Copy link
Contributor

Nice, this would be an incredible way to diagnose what version service teams were running 🙌

I do agree that we shouldn't need to touch ./src files

Since only ./package gets published to npm, could we use pkg.version in these two?

https://github.com/alphagov/govuk-frontend/blob/main/tasks/compile-javascripts.mjs#L7
https://github.com/alphagov/govuk-frontend/blob/main/tasks/compile-stylesheets.mjs#L11

Perhaps we have development as the default in ./src but replace for ./package at build time

:root {
   --govuk-frontend-version: "development";
 }

@querkmachine
Copy link
Member Author

@colinrotherham Would that account for people who import our Sass files directly? That was my main motivation for having the version be part of the Sass, rather than something added during or after CSS compilation.

@colinrotherham
Copy link
Contributor

@colinrotherham Would that account for people who import our Sass files directly? That was my main motivation for having the version be part of the Sass, rather than something added during or after CSS compilation.

Yeah for the govuk-frontend npm package it does

If you click the Code tab you'll only see the npm pack files zipped from ./package

Where all the Sass files have been run through compile-stylesheets.mjs to get vendor prefixes, even though they didn't actually get compiled to CSS (Sass compile to CSS is optional, but PostCSS is mandatory)

@querkmachine
Copy link
Member Author

@colinrotherham I've had a bit of a poke at this on my local development environment but I don't really have enough knowledge or confidence with the current build process to know how to pull those off, short of doing a find-and-replace in the stream during compile—which I don't think is what you had in mind!

As my original comment, happy to pair on this or just toss it over the fence if it's one of those 'it's easier to just do it' things 😋

@colinrotherham
Copy link
Contributor

Wonder if there's a PostCSS plugin that can map JS values into CSS variables?

Or similarly Rollup with { preserveModules: true } could minimally bundle a single version file into the ESM output

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3318 March 6, 2023 12:49 Inactive
@colinrotherham
Copy link
Contributor

Paired up with @querkmachine and we've moved the version code into:

  1. Rollup { plugins: [] } option
  2. PostCSS { plugins: [] } option

For PostCSS we can filter on --govuk-frontend-version but for Rollup via rollup-plugin-replace

@colinrotherham
Copy link
Contributor

@querkmachine I had a quick look and Rollup can parse to an AST via this.parse()

// Add GOV.UK Frontend release version
plugins: [
  {
    name: 'govuk-frontent-version',
    transform: function (code, id) {
      if (id === join(srcPath, 'common/govuk-frontend-version.mjs')) {
        const ast = this.parse(code)

        // Find version export
        const [version] = ast.body
          .flatMap((node) => node.declaration?.declarations ?? [])
          .filter((node) => node.id.name === 'version')

        // Replace version
        if (version) {
          version.init.value = pkg.version

        // TODO: Write AST back to string??
        // https://github.com/estools/escodegen
        // return { ast, code: escodegen.generate(ast), map: null }
        }
      }
    }
  }
]

But you have to generate JavaScript back from the AST yourself in our IE8-friendly Rollup version

May or may not be something newer Rollup handles

@querkmachine
Copy link
Member Author

I'm happy to stick to our current solution for now. Although the find/replace is a little loose, scoping it to only affect the single file we want to change makes it feel fairly safe as-is. The above may be useful if we start including other metadata (like the environment the file was built for) in future, though!

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3318 March 6, 2023 18:00 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3318 March 7, 2023 09:40 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3318 March 7, 2023 11:38 Inactive
colinrotherham
colinrotherham previously approved these changes Mar 7, 2023
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

This was a brilliant idea @querkmachine

✅ All approved from me

You'll see an extra commit to let the webpack example find postcss.config.mjs, but also another commit to remove the webpack-specific workarounds no longer needed

Let me know if you want these in another PR 👍

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Neat solutions! A few minor comments from me, but this is looking really good so far.

tasks/gulp/__tests__/after-build-dist.test.mjs Outdated Show resolved Hide resolved
if (!isDev) {
plugins.push(replace({
include: join(srcPath, 'common/govuk-frontend-version.mjs'),
values: { development: pkg.version }
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be neat if we could also identify pre-releases somehow – could we make it possible to override or append to the version when running npm run build:package, and update the pre-release script to pass the branch name?

Could consider this for a future enhancement if we don't want to do this now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a future enhancement for pre-releases to update package/package.json version?

src/govuk/common/govuk-frontend-version.mjs Show resolved Hide resolved
Comment on lines 60 to 75
Declaration: {
'--govuk-frontend-version': (decl) => {
decl.value = `"${pkg.version}"`
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a short explanation we could add, or some postdoc docs we could link to, to help understand what this is doing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added the following comments with linked I used to find this approach:

// Find CSS declaration for version, update value
// https://github.com/postcss/postcss/blob/main/docs/writing-a-plugin.md
// https://postcss.org/api/#declaration

@querkmachine
Copy link
Member Author

Resolving the govuk-esm version feels like something we should do before releasing this, but the others are hitting me as nice-to-haves, and I'd rather not have this blocked for any longer than necessary (the goal is v4.final, after all!)

@colinrotherham
Copy link
Contributor

@36degrees @querkmachine I've rebased this PR onto:

The same Rollup plugin now runs for package/govuk-esm/common/govuk-frontend-version.mjs

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3318 March 31, 2023 16:53 Inactive
Base automatically changed from rollup-esm to main April 3, 2023 13:56

if (!isDev) {
// Add GOV.UK Frontend release version
// @ts-expect-error "This expression is not callable" due to incorrect types
Copy link
Contributor

Choose a reason for hiding this comment

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

By using "moduleResolution": "Node" we can resolve this in another PR

Although it still works, not all packages are typed to say "I work as ESM-only"

@colinrotherham
Copy link
Contributor

@querkmachine This is all ready to go now, nice work 🙌

@querkmachine
Copy link
Member Author

querkmachine commented Apr 3, 2023

Nice. I'm gonna add some comments to the files, noting that they don't need to be updated manually, then it sounds like we're good to merge.

I don't think we need to add a changelog entry for this. The work to make what version of Frontend is being used visible is very much for our benefit rather than the developers using Frontend, and there are no actions we expect them to make in response to this information now being exposed.

As such I'm thinking we treat it like any internal API change and not record it in the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easier to see which version of Frontend a service is using
4 participants