core: fix NodeDetails type mismatch

[brendankenny]

I was reviewing some code recently, noticed we're being very aggressive in some type casts of audit details to /** @type {LH.Audit.Details.NodeValue} */, removed some of them, and found a type error due to the NodeDetails changes in #11405 that wasn't flagged due to the casts hiding the difference between types (in this case, a null property in the artifact being used for an optional (but not nullable) property in the audit details).

We were using that cast because if strings are nested in an object, they're typically widened by tsc from the literal value to just string, which means they're no longer valid AuditDetails (which need a specific literal). But casts are dangerous (this is a pretty mild example of how they can go wrong), so we should avoid if we can.

There are two main options instead of casting:

1. cast just the string

   type: /** @type {'node'} */ ('node')

   This is annoying, but it's about as painful as in the typescript world (there you could add as const to all of these strings)

2. type the container these are going into.

   /** @type {Array<{node: LH.Audit.Details.NodeValue}>} */
   const items = rule.nodes.map(node => ({
     node: {
       type: 'node',
       // ...
     },
   };
   

   This has improved since we originally added these types, where even if the string literal is several layers deep in an object literal, if it's going into e.g. this example array that's typed with that audit details, the string won't be widened

(2) seems like they better solution, and with details items like /** @type {Array<{node: LH.Audit.Details.NodeValue}>} */ it's really not so bad. But several of these have more properties (href, target, rel...) and so the type starts getting excessive compared to just letting it be inferred.

As a result, I went with option 1, but completely happy to go the other way instead.

Either way, hopefully this will set the template for how to items in future audits and we can stamp out some future casting :)

[brendankenny]

this cast does nothing because the untyped ...getNodeDetails() below is spreading ...any which just spreads into the whole object being any :)

[brendankenny]

This is the functional change to the PR. In practice it shouldn't be observable because I believe null could only ever come from link-elements and script-elements and we wouldn't have been trying to put their boundingRects in the LHR in the first place, but you never know where some code is checking against undefined instead of falsiness or something.

[connorjclark]

I came to the same conclusion, although my analysis only went surface level: Wrapping multi-line object literals in a cast is much more awkward than for a single string. Stylistically alone I prefer doing this for all detail value types.

[brendankenny]

Stylistically alone I prefer doing this for all detail value types

have I got a follow up for you :)