Fix handling of scope being null when calling createAll() or initAll()

[colinrotherham]

Hello from the NHS digital service manual team 👋

We've been doing lots of testing of createAll() and initAll() on optional or dynamically created HTML

This PR fixes a couple of issues affecting scoped initialisation I raised earlier today:

• JavaScript error on createAll() with scope not found #6161
• Entire document is initialised by initAll() when scope not found #6162

We also noticed that createAll() passes configs to non-configurable components so we've fixed this, updated some tests, and I've had a go at the stretch task from @romaricpascal so the compiler can spot this in future in:

• [Stretch] Review the overlap of the different types representing a component class in our code #5469

Happy to tweak things, adjust naming conventions, or talk you through anything 😊

[36degrees]

Hey @colinrotherham 👋🏻

Thanks for taking the time to raise this 🙏🏻

There's quite a lot of changes in this pull request – would you be up for splitting out some of the commits into separate pull requests to make it easier for us to review?

From a quick glance we think:

• b13e197 looks like it could stand alone?
• 5b32c33, cf673b2, 9e20b98 and aeaaff5 look like type changes not directly related to the main change – if that's the case, could they be split out?

We're also not sure about f8384bc – although childConstructor references a class, it is not in itself a class, so we think it'd make sense to leave snake-cased? Happy to discuss if you disagree, but again might be better split out as a separate change.

Finally, having discussed what the behaviour should be if you pass a null scope in our dev catch up – we think that the behaviour of silently not initialising may be unexpected and hard to debug if you e.g. have a typo in your selector.

We think that it'd make more sense to treat it as an error, and either call onError with an ElementError (or console.log it if no onError callback is provided, as we do with SupportError). Would you be OK with that change?

[colinrotherham]

@36degrees Happy to split them out where possible, not a problem at all

I might hit issues trying to do stacked PRs from a fork, unless you know a way?

Standalone console.log spies

b13e197 looks like it could stand alone?

Yeah this commit can stand alone, but do you mind if I include it?

This was because only some of your tests add the spy whereas other test assertions rely on it existing from previous tests. When it came to adding .only() to my new test cases they didn't work as console.log wasn't a mock

Error handling

We think that it'd make more sense to treat it as an error, and either call onError with an ElementError (or console.log it if no onError callback is provided, as we do with SupportError). Would you be OK with that change?

Ahh I'd assumed we'd copy createAll(Button), where no buttons = no errors

But that sounds good, happy to treat it as an error instead

Static methods and properties

Regarding this bit:

We're also not sure about f8384bc – although childConstructor references a class, it is not in itself a class, so we think it'd make sense to leave snake-cased? Happy to discuss if you disagree, but again might be better split out as a separate change.

It was in the sense that static methods and properties are available:

childConstructor.checkSupport()
childConstructor.moduleName

So it seemed odd to use two camelCase names childConstructor (line 44) and constructor (line 86) when familiar class static APIs like Object.entries() or Array.isArray() use PascalCase

Classes versus instances

Part of the type fixes were to distinguish between:

• ComponentClass versus InstanceType<ComponentClass>
  AKA class Accordion {} (class) versus new Accordion() (instance)

Once that was done it fixes the issue in this big comment but now the compiler realises we're passing configs into non-configurable components so aeaaff5 and 9e20b98 might need to all stay together

I'll see what I can do

[36degrees]

It was in the sense that static methods and properties are available:

childConstructor.checkSupport()
childConstructor.moduleName

So it seemed odd to use two camelCase names childConstructor (line 44) and constructor (line 86) when familiar class static APIs like Object.entries() or Array.isArray() use PascalCase

IMO the snake-casing is appropriate here as it's an indicator that there's an extra level of indirection involved – childConstructor is a reference to the constructor, it is not itself a constructor.

When you're calling Object.entries() or Array.isArray() you're calling static methods that exist on the Object and Array class.

When you're accessing the childConstructor.moduleName property you're not accessing a static property on the ChildConstructor class. There is no 'thing' with the name ChildConstructor other than the constant we're creating.

Does that makes sense?

[colinrotherham]

Does that makes sense?

Yeah 100% understand why for both, those two different names caught me out

Unfortunately I've struggled to split this work out 😔

Your ESLint setup skips strictNullChecks so adding | null | undefined types are ignored:

Without strictNullChecks ESLint will often say that checks or fallbacks are unnecessary:

Unnecessary conditional, expected left-hand side of ?? operator to be possibly null or undefined.
eslint(@typescript-eslint/no-unnecessary-condition)

How do you feel about fixing this so ESLint can spot where null has been missed in future?

There are some brilliant updates that are skipped currently and the blog post The Problem With Projects will fix both the null/undefined checks and some slow linting issues you've probably run into:

-   "@typescript-eslint/eslint-plugin": "^7.17.0",
-   "@typescript-eslint/parser": "^7.17.0",
+   "@typescript-eslint/eslint-plugin": "^8.39.0",
+   "@typescript-eslint/parser": "^8.38.0",

Separate PR of course

But any ideas on how to do a stacked PR from a fork?

[colinrotherham]

When you're accessing the childConstructor.moduleName property you're not accessing a static property on the ChildConstructor class. There is no 'thing' with the name ChildConstructor other than the constant we're creating.

Ahh I remember now, because it went to PascalCase again in the Component param name

function createAll(Component, config, createAllOptions) {}

function formatErrorMessage(Component, message) {}

function normaliseDataset(Component, dataset) {}

[colinrotherham]

@36degrees Can you check these commits before I open another PR?

I've made the changes to throw new ElementError() as necessary

[36degrees]

Thanks for doing this @colinrotherham! The changes to throw the ElementError look good.

Can we drop the changes in 529df88? We appreciate you picking that up, but we think we need to handle that refactor ourselves to be sure we completely understand what's going on with our types, and share that knowledge around the team.

The other thing that we're not sure about is the changes in bcdc9d6 – what's the benefit of overloading the function signature in this way, rather than the previous approach? It seems to introduce a lot of repetition and verbosity.

[colinrotherham]

@36degrees Ah I didn't expect you to look at those anymore, I've prepared these commits only and can drop the rest

Yep overloads are definitely super repetitive but they can generate clear documentation on the three distinct ways createAll() can be used

E.g. If you pass in an HTML element to the third parameter, your editor will show $scope (with the scope description) instead of createAllOptions in the tooltip

They're great when one of the combinations needs to be deprecated, as ESLint can tell you which overload to use instead

For example, the 3rd parameter can be an error handler function (not an object) but it isn't documented so maybe that combination should be deprecated?

Happy to drop it though

Update: Dropped

[colinrotherham]

How's it looking @36degrees?

[colinrotherham]

I've rebased this now since #6180 was merged

[36degrees]

Thanks for this @colinrotherham – just noticed a couple of small things that it'd be good to get resolved if you've got time.

[36degrees]

This has the side-effect that initAll can now be called with an HTMLElement that will be used as the scope, or a function that will be used as the onError callback.

Do we need to update the type definition for config to reflect that?

[colinrotherham]

Of course! Yeah we support initAll($scope) in NHS.UK frontend for legacy reasons

If you're happy to have it here too?

[36degrees]

Yeah. It wasn't something we were looking at doing, but I think it makes sense from a consistency point of view and makes it slightly easier to maintain.

[colinrotherham]

Pushed 👍

[colinrotherham]

With those commits we removed on another branch if you need them

[romaricpascal]

Cheers for fixing this and moving the overload code to a separate branch!