Allow CharacterCount component to receive i18n config via JS

[romaricpascal]

Builds upon the configuration work for the CharacterCount component to allow translating the message displaying the count.

To avoid adding another flurry of Puppeteer tests, the PR starts with a little refactoring to isolate the formatting of the message so it can be unit tested. The Puppeteer tests remain in place to keep testing that the right thing appears based on user interactions.

Closes #2805, and closes #2568 as well, and closes #2701 also.

[36degrees]

I'm not 100% sure that we need this abstraction, and it could end up trying to do too much if we extend it to do everything we might want to do with a created element as the TODOs are hinting at.

Are we sure we want to go down this route rather than just using the native APIs directly in the tests?

[romaricpascal]

Issue created for discussion there: #2894

[36degrees]

Slightly nervous about passing a div here, and imagine this could come back to haunt us in the future when e.g. we get rid of the init function (which at the minute we're not calling, so a lot of the 'setup' for the component isn't happening).

Don't have any immediate suggestions for a fix, and this might just be something we have to revisit later… but it feels a bit fragile.

[romaricpascal]

Because we have the Puppeteer tests, I'm not too worried about mocking what the component runs onto and skipping its initialisation.

If anything, having these tests only depend on the bare minimum that the feature needs (one element and no init call) can help us ensure we do update tests: if the formatting starts to depend on more than what's mocked, these tests will fail and force us to update them or to better isolate that method, and maybe prompt us to add new cases around the change that we may have missed through the end-to-end tests in Puppeteer (because we can't cover all combinations of everything there, while we can be more thorough in unit tests which run with less overhead).

Is it the extra updates that worry you?

[36degrees]

As flagged in Slack, although document seems to be part of the default globals for Standard, we might want to explicitly configure eslint so that it knows this is file is in the context of a browser / jsdom (for example by adding another override for this file in .eslintrc.js)

[romaricpascal]

If we go down the route of finely tracking which environment which file is in, I feel it's something that should happen across the whole codebase in a separate PR, otherwise we'll have parts of the code still able to access the browser globals when they shouldn't.

That said, as I mentioned on Slack, I'm not sure it's super useful to track browser globals with that granularity:

• our project does run in the browser after all
• any code that doesn't run in the browser will either run locally or on CI first and we can uncover access to non existing globals there.

[romaricpascal]

@36degrees Made the updates based on your last feedback. Happy to discuss test and linting concerns further but I feel they're two things that could be fixed in future PRs rather than completely blocking that one from moving forward?

[colinrotherham]

@36degrees Made the updates based on your last feedback. Happy to discuss test and linting concerns further but I feel they're two things that could be fixed in future PRs rather than completely blocking that one from moving forward?

@romaricpascal I've found it helps to ask if anything has changed "for the worse" in the PR

If it's something we agree we can live with temporarily, fine. Otherwise if the changes have carried on using existing conventions an techniques, code style etc, improvements can come later

[colinrotherham]

@romaricpascal I'll add my thoughts for the future here

Looking at the dom-helpers.js module (similar to puppeteer-helpers.js that I added) perhaps we should extend the Jest environments instead, not yet but soon?

Otherwise could it grow into a "mini jQuery"?

We already use cheerio for this elsewhere. Faster to use a library that to write our own utilities.

Have you thought about adding these at environment level? Like the Puppeteer tests having the page global

Jest environments

Since Jest doesn't implement require.cache you'll notice that any files we require/import in tests (versus environments) are executed hundreds of times—this does start to add up when you're looping directory listings 😮

1. Environment @jest-environment jsdom
   Could we extend this to always require() cheerio global.$ so dom-helpers.js isn't needed?

2. Environment @jest-environment puppeteer
   Could we extend this to always require() the various helpers like renderAndInitialise, waitForHiddenSelector, waitForVisibleSelector so they're also available in tests automatically?

[romaricpascal]

@romaricpascal I've found it helps to ask if anything has changed "for the worse" in the PR

Oh, that's a good one, I'll keep it in mind. If things are moving "for the better", we can always move them further "for the better" in future PRs indeed.

If it's something we agree we can live with temporarily, fine. Otherwise if the changes have carried on using existing conventions an techniques, code style etc, improvements can come later

I think that words what I'm trying to get: a confirmation that we'd be fine shipping as is and revisiting in the future if we want to amend tests/linting

Looking at the dom-helpers.js module (similar to puppeteer-helpers.js that I added) perhaps we should extend the Jest environments instead, not yet but soon?

I have a slightly different direction in mind for these helpers, where we could also use them to create elements in the components themselves. Unlike the querying, the DOM API is very verbose when it comes to creating elements with attributes and children and I think we'd gain some readability with a little abstraction there. I've opened an issue to discuss there: #2894.

Otherwise could it grow into a "mini jQuery"?

Yeah, that's something we'd need to keep an eye on if we move these helpers further.

We already use cheerio for this elsewhere. Faster to use a library that to write our own utilities.

Cheerio didn't quite fit the bill there as we needed to get an instance of HTMLElement to run the component onto without spinning up an actual browser. I didn't see any API in Cheerio to get something like that out. It seems to only have those jQuery-esque objects or text outputs.

Jest environments

Since Jest doesn't implement require.cache you'll notice that any files we require/import in tests (versus environments) are executed hundreds of times—this does start to add up when you're looping directory listings 😮

1. **Environment `@jest-environment jsdom`**
   Could we extend this to always `require()` **cheerio** `global.$` so **dom-helpers.js** isn't needed?

2. **Environment `@jest-environment puppeteer`**
   Could we extend this to always `require()` the various helpers like `renderAndInitialise`, `waitForHiddenSelector`, `waitForVisibleSelector` so they're also available in tests automatically?

Wasn't aware of the lack of require caching in Jest, cheers for pointing it out 😄 I'm wondering how much of a bottleneck it is vs how much we'd lose of which files rely on which helpers that import bring. I'd personally err on the cautious side of not putting too many globals if we don't have perfomance issues, but it may be an improvement worth tracking in an issue 😄

[colinrotherham]

Ahh fair enough, I thought Cheerio had jQuery's $('<p>hello</p>') quick creation

Should dom-helpers.js be moved inside ./src/govuk/?

[romaricpascal]

Ahh fair enough, I thought Cheerio had jQuery's $('<p>hello</p>') quick creation

It does, but the returned object doesn't have an equivalent of jQuery's .el() to return the corresponding HTML element from what I gathered of their docs. It's text -> jQuery-like object -> text/html/xml. They highlight in their docs that they are "not a DOM emulation project like JSDom"

Should dom-helpers.js be moved inside ./src/govuk/?

Not yet, it is deliberately in lib to highlight that it's tooling code rather than frontend code. It's also using syntax and element (template) not compatible with the browsers we supports 😄

[colinrotherham]

See what @36degrees thinks?

We should do the hard work (+ tests) via a nice neat DOM helpers file, but only if it was going in ./src/govuk/?

Until that happens, if it's just for some some quick ad-hoc test utilities we can use shorthand? Need to be aware of the subtle differences between properties/attributes, but these do the job:

Adding [lang] to <div>

const $localised = Object.assign(document.createElement('div'), {
  lang: 'de'
})

Adding content to <div>

const $content = Object.assign(document.createElement('div'), {
  innerHtml: 'Hello World'
})

Finding closest [lang] attribute value

$closest = $element.closest('[lang]')?.getAttribute('lang')

[colinrotherham]

Super minor one, but any thoughts on logically grouping what the test is doing?

const $parent = document.createElement('div')
const $div = document.createElement('div')

$parent.setAttribute('lang', 'de')
$parent.appendChild($div)

const component = new CharacterCount($div)
const countMessage = component.formatCountMessage(10000, 'words')

expect(countMessage).toEqual('You have 10.000 words remaining')

1. Create things
2. Customise things
3. Run some code
4. Run assertion

Helps with readability slightly, not a blocker though

[colinrotherham]

Some minor comments, but looks fab so approving

I'll leave it with you 😊

Update: Oh and the conflict, quick rebase and we'll see if everything passes again?