misc: tighten RecursivePartial type

[brendankenny]

Fixes an issue with RecursivePartial<T> and simplifies its implementation.

This came up when updating to tsc 3.9.7 in #11158 and I started playing with this @ts-expect-error (previously @ts-ignore):

lighthouse/lighthouse-core/test/gather/gather-runner-test.js

Lines 52 to 57 in cc5c795

	/**
	* @param {RecursivePartial<LH.Config.Json>} json
	*/
	function makeConfig(json) {
	// @ts-expect-error: allow recursive partial.
	const config = new Config(json);

LH.Config.Json is mostly optional by design, so it seemed like it shouldn't be too hard to make it work without the ignore (I failed, but let's ignore that, too :).

The current RecursivePartial was recursing into arrays and making the elements optional, which seems ok at first glance but it's almost never what you actually want. If you ask for an array with missing elements you (almost always) expect a shorter array, not a holey array (i.e. RecursivePartial<Array<string>> should be Array<string>, not Array<string | undefined>).

As a more concrete example, that @ts-expect-error line was silencing a number of complaints about making LH.Config.Json recursively optional, e.g. for SharedFlagsSettings.output, an allowed optional form of Array<'json'|'html'|'csv'> would be [undefined, undefined, undefined], which would be totally broken.

Not necessarily a huge deal for test code, but we do use it in non-test code in config.js (and it's there for more use in the future), so it's worth keeping it in working order.

This PR also simplifies RecursivePartial. As long as we keep the conditional type distributing across unions correctly, they don't need to be split up into tuples and merged back again.

I wasn't there for the discussions in #10123 or #10215, so cc @connorjclark and @patrickhulce to make sure all the use cases from those PRs are still covered (AFAICT they are).

[brendankenny]

I wasn't able to make RecursivePartial<LH.Config.Json> work, but LH.Config.Json is so close to fully optional that the only thing needed to be used directly was sprinkling in a few passNames in the test configs below. Since it's such a small amount of extra setup and we did make passName required on purpose, it seemed like a good tradeoff to make.

[brendankenny]

this does treat tuples the same as arrays, where sometimes you really might always want an array of a certain length (so you use a tuple for the type) and it's ok if it's ['Document', undefined, undefined, 'Script'] in test code. That seemed unlikely enough that I didn't do it here to keep things simple, but something like the following would catch most of those cases:

type RecursivePartialArrayOrTuple<T> = T extends (infer U)[] ?
  number extends T['length'] ?
    // Array or unbounded-length tuple.
    RecursivePartial<U>[] :
    // Bounded-length tuple.
    {[P in keyof T]?: RecursivePartial<T[P]>} :
  never;

That does still treat unbounded-length tuples (e.g. [string, boolean, ...number[]]) like arrays and not tuples, but I'm not sure if there is a way to tell the difference in a conditional type.

[patrickhulce]

a few questions probably because I misunderstood the need for our massive RecursivePartial in the first place

[patrickhulce]

Hey, now here's a type I can understand 😆

I'm curious how this solves the original type Foo = {foo: Array<T> | null} problem though...

As long as we keep the conditional type distributing across unions correctly,

Where does that happen? Does typescript do this for us magically somehow? AFAICT this just applies to arrays and straight up objects and everything else just returns T, so wouldn't RecursivePartial<Foo> just be {foo?: Array<T> | null} instead of {foo?: Array<RecursivePartial<T>> | null}?

they don't need to be split up into tuples and merged back again.

Hallelujah 🎉

[brendankenny]

Does typescript do this for us magically somehow?

Basically, yes, but in a mostly good way :) I should probably have explained why that works now, though.

They define it kind of poorly, but as long as the type parameter is used directly as part of the check, the conditional type will distribute across unions.

type Foo = {foo: Array<T> | null}
RecursivePartial<Foo>

So (if working correctly):

• Foo goes into the object branch, Foo.foo is made optional by the ?:, then Foo.foo's type is passed into RecursivePartial.
• RecursivePartial<Array<T> | null> would distribute and be treated as RecursivePartial<Array<T>> | RecursivePartial<null>, giving Array<RecursivePartial<T>> | null.

In this case the previous code was checking the conditionals against T[P], which was preventing distribution. By checking directly against T instead (so all the T[P] checks move one layer deeper and are themselves properly distributed since they're also looked at individually), it's distributive again.

[brendankenny]

The other type of conditional that breaks distribution is if the type parameter isn't the thing being checked against.

The RecursivePartialArrayOrTuple I suggested below would fail both checks because it's doing number extends T['length'] ?. T isn't being used directly and it's number that's been checked, not the type parameter (to check for bounded vs unbounded lengths).

So if we do end up using something like that, it should only be called by RecursivePartial, which would have already done the distributing and can call RecursivePartialArrayOrTuple on what it knows are non-union types.

[patrickhulce]

gotcha, yeah that makes sense 👍

I guess I never really questioned Partial<ObjA | ObjB> distribution either but that's probably only because I never saw a 50 line implementation of it that I thought was necessary :)

[patrickhulce]

LGTM 🎉

[connorjclark]

💀