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 :)