Fix invalid CSS; add assertions that CSS is valid

[ob6160]

What is the purpose of this change?

In some cases, Emotion CSS syntax has started to slip into source-foundations.

This means that the latest version of foundations is breaking the build in projects like dotcom rendering (AMP specifically) when upgrading the version where the CSS blocks that we export are being rendered directly to the page.

This change fixes some invalid CSS comments that use // vs /* .. */ in our reset which could affect the cascade and adds unit tests which make an assertion using the lightningcss parser so we have confidence that we are shipping valid CSS.

We also clarify the use of the parent selector in resets.input and in the Typography module — by making two exports for each font type. The difference is explained below:

• headline.medium() — This will now return a SerializedStyles object, because it returns a parent selector. It is a breaking change to the API because this used to return an invalid CSS string.
• headlineString.medium() — This will return a CSS string as before, but without including the parent selector.

These assertions are implemented as a custom Jest matcher called toBeValidCSS, which accepts an option for when the CSS to check is a fragment and not surrounded by a selector.

I was wary of adding a new dependency here, but decided to include this in the pull request proposal because it seems very easy to let invalid CSS syntax slip into what is (I think at least?) a package that should be framework agnostic.

Thanks @SiAdcock & @joecowton1 for a great discussion around this!

[changeset-bot]

🦋 Changeset detected

Latest commit: eefed91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@guardian/source-foundations	Major
@guardian/eslint-plugin-source-foundations	Major
@guardian/source-react-components-development-kitchen	Major
@guardian/source-react-components	Major
@guardian/eslint-plugin-source-react-components	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

yarn.lock changes

Click to toggle table visibility

Name	Status	Previous	Current
detect-libc		-	1.0.3
lightningcss-darwin-arm64		-	1.16.0
lightningcss-darwin-x64		-	1.16.0
lightningcss-linux-arm-gnueabihf		-	1.16.0
lightningcss-linux-arm64-gnu		-	1.16.0
lightningcss-linux-arm64-musl		-	1.16.0
lightningcss-linux-x64-gnu		-	1.16.0
lightningcss-linux-x64-musl		-	1.16.0
lightningcss-win32-x64-msvc		-	1.16.0
lightningcss		-	1.16.0

[ob6160]

Fix invalid CSS syntax exported from source-foundations

[mxdvl]

This is a great catch. I have an issue with the fact that this relies on a specific library’s behaviour when parsing CSS.

We want to avoid early abstraction, but as we are already using the transform four times already, I would strongly suggest creating a helper method like isValidCSS that is tested with valid and invalid inputs. While I am quite familiar with lightningcss having used if very recently, I think that the indirection of creating a buffer, specifying a filename and expecting errors to be thrown makes this code harder to maintain.

[sndrs]

are we deprecating the name, the language or the functionality here?

i.e. if this become pure css, what is deprecated about this?

i wonder if we should rename this to rename this to inputSCSS and re-export it as a deprecated input with the deprecation message?

Suggested change

	/**
	* @deprecated this uses the SCSS parent selector. In the future we
	* should look to make this just CSS or move into into our main reset.
	*/
	// remove styling of invalid input elements that gets applied in Firefox
	const input = `
	/**
	* @deprecated use `resets.inputSCSS`
	*/
	const input = inputSCSS;
	// remove styling of invalid input elements that gets applied in Firefox
	const inputSCSS = `

that effectively adds a new export (resets.inputSCSS) while flagging up the old, ambiguous name as something we want to remove.

that would then decouple the misleading name issue from any future work we might want to do on making the reset available as pure css.

[ob6160]

Agreed, that’s a lot clearer - thanks for the suggestion!

[ob6160]

As discussed yday - I've changed input to just have the type: SerialisedStyles which should indicate this properly

[ob6160]

Closing to reopen as two distinct changes, one for the CSS validity assertions and another for our Emotion exports

[ob6160]

Fix invalid CSS syntax exported from source-foundations