Skip to content

misc: tighten RecursivePartial type #11175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 28, 2020
Merged

misc: tighten RecursivePartial type #11175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,9 @@ const GatherRunner = {
};

/**
* @param {RecursivePartial<LH.Config.Json>} json
* @param {LH.Config.Json} json
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
function makeConfig(json) {
// @ts-expect-error: allow recursive partial.
const config = new Config(json);

// Since the config is for `gather-runner`, ensure it has `passes`.
Expand Down Expand Up @@ -214,7 +213,7 @@ describe('GatherRunner', function() {
it('collects network user agent as an artifact', async () => {
const requestedUrl = 'https://example.com';
const driver = fakeDriver;
const config = makeConfig({passes: [{}]});
const config = makeConfig({passes: [{passName: 'defaultPass'}]});
const options = {requestedUrl, driver, settings: config.settings};

const results = await GatherRunner.run(config.passes, options);
Expand All @@ -229,7 +228,7 @@ describe('GatherRunner', function() {
return Promise.resolve({finalUrl, timedOut: false});
},
});
const config = makeConfig({passes: [{}]});
const config = makeConfig({passes: [{passName: 'defaultPass'}]});
const options = {requestedUrl, driver, settings: config.settings};

return GatherRunner.run(config.passes, options).then(artifacts => {
Expand Down Expand Up @@ -1361,6 +1360,7 @@ describe('GatherRunner', function() {
];
const config = makeConfig({
passes: [{
passName: 'defaultPass',
gatherers: gatherers.map(G => ({instance: new G()})),
}],
});
Expand Down Expand Up @@ -1417,6 +1417,7 @@ describe('GatherRunner', function() {
const gathererNames = gatherers.map(gatherer => gatherer.instance.name);
const config = makeConfig({
passes: [{
passName: 'defaultPass',
gatherers,
}],
});
Expand Down Expand Up @@ -1464,7 +1465,10 @@ describe('GatherRunner', function() {
];

const config = makeConfig({
passes: [{gatherers}],
passes: [{
passName: 'defaultPass',
gatherers,
}],
});

/** @type {any} Using Test-only gatherers. */
Expand Down Expand Up @@ -1550,6 +1554,7 @@ describe('GatherRunner', function() {

const config = makeConfig({
passes: [{
passName: 'defaultPass',
gatherers: [{instance: new WarningGatherer()}],
}],
});
Expand Down Expand Up @@ -1603,6 +1608,7 @@ describe('GatherRunner', function() {
const gathererNames = gatherers.map(gatherer => gatherer.instance.name);
const config = makeConfig({
passes: [{
passName: 'defaultPass',
gatherers,
}],
});
Expand Down
55 changes: 7 additions & 48 deletions types/externs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,6 @@
import _Crdp from 'devtools-protocol/types/protocol';
import _CrdpMappings from 'devtools-protocol/types/protocol-mapping'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💀

// Convert unions (T1 | T2 | T3) into tuples ([T1, T2, T3]).
// https://stackoverflow.com/a/52933137/2788187 https://stackoverflow.com/a/50375286
type UnionToIntersection<U> =
(U extends any ? (k: U)=>void : never) extends ((k: infer I)=>void) ? I : never

type UnionToFunctions<U> =
U extends unknown ? (k: U) => void : never;

type IntersectionOfFunctionsToType<F> =
F extends { (a: infer A): void; (b: infer B): void; (c: infer C): void; (d: infer D): void; } ? [A, B, C, D] :
F extends { (a: infer A): void; (b: infer B): void; (c: infer C): void; } ? [A, B, C] :
F extends { (a: infer A): void; (b: infer B): void; } ? [A, B] :
F extends { (a: infer A): void } ? [A] :
never;

type SplitType<T> =
IntersectionOfFunctionsToType<UnionToIntersection<UnionToFunctions<T>>>;

// (T1 | T2 | T3) -> [RecursivePartial(T1), RecursivePartial(T2), RecursivePartial(T3)]
type RecursivePartialUnion<T, S=SplitType<T>> = {[P in keyof S]: RecursivePartial<S[P]>};

// Return length of a tuple.
type GetLength<T extends any[]> = T extends { length: infer L } ? L : never;

declare global {
// Augment Intl to include
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/getCanonicalLocales
Expand All @@ -51,30 +27,13 @@ declare global {
};

/** Make optional all properties on T and any properties on object properties of T. */
type RecursivePartial<T> = {
[P in keyof T]+?:
// RE: First two conditions.
// If type is a union, map each individual component and transform the resultant tuple back into a union.
// Only up to 4 components of a union is supported (all but the last few are dropped). For more, modify the second condition
// and `IntersectionOfFunctionsToType`.
// Ex: `{passes: PassJson[] | null}` - T[P] doesn't exactly match the array-recursing condition, so without these first couple
// conditions, it would fall through to the last condition (would just return T[P]).

// RE: First condition.
// Guard against large string unions, which would be unreasonable to support (much more than 4 components is common).

SplitType<T[P]> extends string[] ? T[P] :
GetLength<SplitType<T[P]>> extends 2|3|4 ? RecursivePartialUnion<T[P]>[number] :

// Recurse into arrays.
T[P] extends (infer U)[] ? RecursivePartial<U>[] :

// Recurse into objects.
T[P] extends (object|undefined) ? RecursivePartial<T[P]> :

// Strings, numbers, etc. (terminal types) end here.
T[P];
};
type RecursivePartial<T> =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Recurse into arrays and tuples: elements aren't (newly) optional, but any properties they have are.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

T extends (infer U)[] ? RecursivePartial<U>[] :
// Recurse into objects: properties and any of their properties are optional.
T extends object ? {[P in keyof T]?: RecursivePartial<T[P]>} :
// Strings, numbers, etc. (terminal types) end here.
T;

/** Recursively makes all properties of T read-only. */
export type Immutable<T> =
Expand Down