Add postcss.config.js/.browserslistrc configs

[colinrotherham]

This PR adds two new config files postcss.config.js and .browserslistrc

PostCSS config via postcss.config.js

To make it simpler to configure our CSS output the following postcss() calls now share the same config

• Adding CSS prefixes to Sass files in ./package
• Autoprefixer + CSSNANO for ./dist output
• Autoprefixer + CSSNANO + "Old IE" for ./dist (IE 8 styles)
• Autoprefixer + 'companion' pseudo-selector classes for ./public
• Autoprefixer + 'companion' pseudo-selector classes for ./public (Legacy styles)
• Autoprefixer + 'companion' pseudo-selector classes + "Old IE" for ./public (Legacy + IE8 styles)
• Autoprefixer + "Old IE" for ./public (IE 8 styles)

Browser config via .browserslistrc

It's nice to have a separate shareable config which could also be used by Babel, Jest, ESLint, in future

Breaking changes have been moved into a second PR #2924

[36degrees]

Did you see the investigation I did into which plugins were actually doing something we needed?

I think we can probably get rid of calc, unroot, unnth and potentially unnot as when I last tested they weren't doing anything.

We might also actually need postcss-unmq – although we have unmq: { disable: true } in the config, it's using the wrong namespace so that bit of the config isn't actually doing anything 🙃

Generally it'd be good to compare the built dist and package folders with and without this change to see what effect it has.

[colinrotherham]

Did you see the investigation I did into which plugins were actually doing something we needed?

I did indeed, was very useful (stuck it in the description!)

This PR was more of a like-for-like "as is" spike, just to see if it was possible

Shall I add the workflow_dispatch events (run manually) to Diff changes to dist like we did here? 396697a

[colinrotherham]

Soon as I add postcss-unmq this error from @lfdebrux comes back:

Unknown error from PostCSS plugin. Your current PostCSS version is 8.4.16, but postcss-unmq uses 5.2.18. Perhaps this is the source of the error below.
Cannot read property '1' of null

But the error comes from css-mediaquery (See issue: ericf/css-mediaquery#7)

If we tap into their code and add some logging it's caused by:

https://github.com/alphagov/govuk-frontend/blob/main/src/govuk/helpers/_device-pixels.scss#L33
https://github.com/alphagov/govuk_frontend_toolkit/blob/master/stylesheets/_device-pixels.scss#L4

{
  expression: '(-o-min-device-pixel-ratio: 20 / 10)',
  RE_MQ_EXPRESSION: /\(\s*([^\s\:\)]+)\s*(?:\:\s*([^\s\)]+))?\s*\)/,
  captures: null
}

☝️ It was never expecting media queries to be formatted that way

But why??

The older [email protected] is removing -o-min-device-pixel-ratio
The older [email protected] is preserving -o-min-device-pixel-ratio

Changes via npx browserslist
Querying browserslist for changes only added firefox 105 and modified:

and_qq 10.4 → and_qq 13.1
and_uc 12.12 → and_uc 13.4

Just a hunch (similar to Babel), but I wonder if node_modules Sass imports are excluded from processing now

[colinrotherham]

Solved it! Had another go at this during the Frontend Community meeting 😊

We were running the autoprefixer() plugin with our usual browsers for the IE8 stylesheet

> 0.1%
last 2 Chrome versions
last 2 Firefox versions
last 2 Edge versions
last 2 Samsung versions
Safari >= 9
ie 8-11
iOS >= 9

But for the IE8 styles we guard with <!--[if lte IE 8]> yet still added -webkit-, -ms prefixes etc

For our Browserslist config I've added a lonely new environment:

[oldie]
ie 8

So this PR now introduces two "diffs" versus the previous CSS output:

1. Unnecessary prefixes for IE8 styles are gone
2. Selectors for :not rulesets are back as mentioned by @36degrees

Ready for review 👍

[colinrotherham]

The usual env is "production" or "development" based on NODE_ENV

[colinrotherham]

This is another one linked with #2243

[colinrotherham]

Breaking changes moved to #2924

[colinrotherham]

With the PostCSS configs separated out the Gulp task can be simplified too #2912

[domoscargin]

Neat!

[colinrotherham]

Due to tooling file extension differences (source vs target) this is now name.endsWith('.min')

[domoscargin]

The original logic for this looks like it was "anything that's not the dist folder".

I get the other two if conditions, but for some reason I'm struggling to figure this change out, so I've probably missed something obvious - could you explain why this only applies to app- files?

[domoscargin]

Ah, no, I should've read the surrounding code - so this stuff is applied to:

app/assets/scss/app.scss in compileStyles
app/assets/scss/app-legacy.scss in compileLegacy
app/assets/scss/app-legacy-ie8.scss in compileLegacyIE.

So my revised query is why app.scss isn't included in the if condition?

[colinrotherham]

You beat me to it with your reply!

I started by spotting the companion classes added in three places:

• compile-assets.js#L60
• compile-assets.js#L118
• compile-assets.js#L133

When taking only !isDist into account that gives us the filenames:

app.css
app-legacy.css
app-legacy-ie8.css

Strangely you'll see app-ie8.scss was excluded (a bug?) so that's now fixed

BUT brilliant spot though. I'm going to push again to include app.css 🤦‍♂️

[colinrotherham]

I've pushed again due to a slight complication

1. Plugin for Gulp gulp-postcss populates PostCSS ctx.file as type import('vinyl')
2. Plugin for webpack postcss-loader populates PostCSS ctx.file as a file path string

Found this out using the webpack example #2869

Additionally the Gulp stream ctx.file.extname will be .css but other tools pass the original .scss

Take a look at the new version 👍

Thanks @domoscargin

[36degrees]

I don't think this is doing what we want when running npm run build:dist because at that point we're generating a minified CSS file which ends with -ie8.min, so this plugin doesn't get added.

[colinrotherham]

Well spotted, this was a well-intentioned change from yesterday (previously .includes()) so I'll put it back!

[colinrotherham]

Done. For clarity I've matched on both:

if (name.endsWith('-ie8') || name.endsWith('-ie8.min')) {

Plus I've added tests for each combination we rely on

[36degrees]

Really nice to have these tests 👍🏻

Just a suggestion, but there's quite a lot of repetition in the test structure – could be a good case for describe.each using something like this?

const combinationsToTest = [
  {
    name: 'Default',
    paths: ['example.css', 'example.scss'],
    expectedPlugins: ['autoprefixer']
  }
]

[colinrotherham]

Thanks @36degrees

Yeah I didn't optimise by choice, but can come back and improve these in future?

Take a look in #2924 (based on this branch) where we start to add in the environment switching tests for "Old IE"

[36degrees]

We also have engines in our package.json – do we need both? Just seems like another thing to potentially get out of sync.

[colinrotherham]

We can remove engines from package.json to be fair

They have different purposes:

1. .browserslistrc → Target environment
   Tells our tooling what features are supported when building

2. Package engines → Development environment
   Tells our local environment what versions are allowed for development

For example, when Node.js 18 LTS lands on the 25th October we'll likely want to increase our engines ranges, update tests, GitHub workflows etc, so our contributors can use the latest supported Node.js version.

But for compatibility, any compiled code (Node.js or browser) should still adhere to our build target environments