feat: Introduce `PickType` #92

untio11 · 2024-06-26T09:24:02Z

New pick type. Distributes over top level unions.

Still need to clean up unit tests. Also picking from a top level union isn't supported properly yet.

src/types/index.ts

src/types/pick.test.ts

pavadeli · 2024-06-26T13:19:53Z

src/types/pick.test.ts

+            { a: 'drop', b: true, c: 2, d: 'tnsh', e: 2 },
+            { a: 'drop', d: 'tnsh' },
+        ],
+        [{ a: 'tns' }, { a: 'tns' }],


This is already checked as part of the validValues.

pavadeli · 2024-06-26T14:10:04Z

src/types/pick.test.ts

+                `errors in [Pick<NastyType, 'optionalUnion' | 'arr'>]:`,
+                `- at <optionalUnion>: expected an object, got a number (13)`,
+                `- at <arr>: expected a [nonempty], got: []`,


Hint, this fails on newlines, but those are presented poorly in the Jest output:

Suggested change

`errors in [Pick<NastyType, 'optionalUnion' | 'arr'>]:`,

`- at <optionalUnion>: expected an object, got a number (13)`,

`- at <arr>: expected a [nonempty], got: []`,

`errors in [Pick<NastyType, 'optionalUnion' | 'arr'>]:`,

'',

`- at <optionalUnion>: expected an object, got a number (13)`,

'',

`- at <arr>: expected a [nonempty], got: []`,

src/types/pick.test.ts

src/types/pick.ts

pavadeli · 2024-06-26T14:48:45Z

src/types/pick.ts

+    protected override typeValidator(input: unknown, options: ValidationOptions): Result<ResultType> {
+        if (!unknownRecord.is(input)) {
+            return this.createResult(input, undefined, { kind: 'invalid basic type', expected: 'object' });
+        }
+
+        const constructResult: Record<string, unknown> = {};
+        const details: MessageDetails[] = [];
+        for (const [key, innerType] of this.propsArray) {
+            const missingKey = !hasOwnProperty(input, key);
+            const partialKey = this.propsInfo[key]?.partial;
+
+            if (missingKey) {
+                partialKey || details.push({ kind: 'missing property', property: key, type: innerType });
+                continue;
+            }
+
+            const innerResult = innerType.validate(input[key], options);
+            if (innerResult.ok) {
+                constructResult[key] = innerResult.value;
+            } else {
+                details.push(...prependPathToDetails(innerResult, key));
+            }
+        }
+        return this.createResult(input, options.mode === 'construct' ? constructResult : input, details);
+    }


This method feels kind of familiar, doesn't it? This is currently great to prove the concept. One of the things you had to change here compared to the implementation in InterfaceType is that "partial" is now an aspect of a single property, instead of the entire type.

I think we should consider refactoring InterfaceType to support that as well. All the ingredients are already in place. This could have the following benefits: (I haven't tried it though)

No more need for a seperate PickType, we can now delegate entirely to InterfaceType (sometimes combined with a UnionType when the input type is a union) from within the pick function

withOptional then returns an InterfaceType instead of an IntersectionType makes sense I guess?¹

we can support config options like strictMissingKeys etc

If we decide to do that, we should do so in another branch. Let's discuss that offline.

Footnotes

Currently, objects with both mandatory and optional properties are implemented as an intersection of one object with the mandatory properties and one object with the optional properties. ↩

Nieuwe issue voor aangemaakt #93

pavadeli · 2024-06-26T14:49:56Z

src/types/pick.ts

+        return this.createResult(input, options.mode === 'construct' ? constructResult : input, details);
+    }
+    override accept<R>(visitor: Visitor<R>): R {
+        // TODO: Should be fine, right?


Yeah, once you distribute over unions in the pick function, then all that remains here is object-like types.

Pick distribueert nu over unions

untio11 · 2024-07-03T10:32:31Z

src/types/pick.ts

+        const innerTypes = unionType.types
+            .map((innerType, idx) => {
+                const specificKeys = narrowedKeys[idx] ?? [];
+                const keyCount = specificKeys.length;
+                // Only build the pick union elements if at least one of their keys has been picked.
+                // Also temporarily store the key count in a tuple to sort.
+                return [keyCount, keyCount ? new PickType(innerType, checkOneOrMore(specificKeys)) : undefined] as const;
+            })
+            // Sorting is necessary so the union type will internally match with a maximum subtype. This is so that types don't accidentally
+            // get narrowed too much during parsing and stringifying.
+            .sort(([aKeys, _1], [bKeys, _2]) => bKeys - aKeys) // Reverse sort: from most keys to least keys.
+            .flatMap(([_, innerType]) => innerType ?? []);


Feels kinda nasty, but I didn't want to touch the implementation of UnionType to deal with finding the maximum pick type.

UnionType seems to just find the first matching type when parsing and stringifying, so when that happens to be a narrower type in the union, you get unexpected results (see the Pick<UnionAC, 'a' | 'd'> test case). The second conversion would also strip d, which is weird. Same would happen with stringify.

By sorting this way, we guarantee that we don't strip too much of the properties accidentally as it will always find a biggest match first.

Interesting idea. If we do something like this, it should be part of the UnionType implementation. I agree that this is a major footgun, so I'm currently considering starting with a pick operation on simpler interface types only.

I need to think and test your idea and possible other solutions to this footgun. We have had situations in our codebase where this happened, so I'm really happy if we can find a way to prevent "accidents" because of this.

untio11 · 2024-07-03T10:49:01Z

src/types/pick.ts

+        return union(
+            // Manually rename the type, because it's confusing if your union elements are suddenly in a different order.
+            name ?? `Pick<${baseType.name}, ${keys.map(key => `'${key}'`).join(' | ')}>`,
+            checkOneOrMore(innerTypes),


Yeah so for example:

const A = object('A', { a: string }); const B = object('B', { a: string, b: boolean, d: undefined }); const C = object('C', { a: string, b: boolean, c: null }); const OriginalUnion = union([A, B, C]); // OriginalUnion.name === 'A | B | C' const PickedUnion = pick(OriginalUnion, ['a', 'b', 'd']); // Because of the sorting, PickedUnion.name would be: // Pick<B, 'a' | 'b' | 'd'> | Pick<C, 'a' | 'b'> | Pick<A, 'a'> // Not wrong per se, just a bit odd that the elements are suddenly in a different order // With the current implementation, the name will look like this instead: // Pick<A | B | C, 'a' | 'b' | 'd'>

Still a bit in doubt about this, as the first name does show the distributive property much clearer.

untio11 · 2024-07-03T10:50:45Z

src/types/pick.ts

+    return createType(new PickType(baseType, keys, name));
+}
+
+export const pickPropertiesInfo = (propsInfo: PropertiesInfo, keys: OneOrMore<keyof Properties>) =>


Exporting these and the helper functions below only for testing, but that feels a bit odd. Is there a better way?

untio11 · 2024-07-03T10:51:15Z

src/types/union.ts

@@ -178,7 +178,7 @@ function analyzePossibleDiscriminators(
    return found ? [...found.values()] : [];
 }

-function propsInfoToProps(propsInfo: PropertiesInfo): Properties {
+export function propsInfoToProps(propsInfo: PropertiesInfo): Properties {


This one I'm using in pick now, but also don't know if it's okay to export just like that.

pavadeli · 2024-10-15T14:06:30Z

Awesome work @untio11! You inspired me to take on #93 to make utility functions like this much easier in the future. Because of the issues with UnionType that you mentioned, I'm inclined to start with a smaller target for the pick operation. I distilled the most important tests, features and lessons learned from this PR and applied them to InterfaceType types. I'll add you as co-author on that PR. 🙏🏼

untio11 added 2 commits June 3, 2024 15:16

Initial implementation working

cbd593a

Close to done

915ffb0

Still need to clean up unit tests. Also picking from a top level union isn't supported properly yet.

untio11 requested a review from pavadeli June 26, 2024 09:27

pavadeli reviewed Jun 26, 2024

View reviewed changes

untio11 mentioned this pull request Jul 1, 2024

Refactor InterfaceType to support partial at the property level #93

Closed

untio11 added 2 commits July 1, 2024 15:40

Non-unit tests comments verwerkt

1661465

Pick distribueert nu over unions

Finished cleaning up

e571626

untio11 commented Jul 3, 2024

View reviewed changes

untio11 marked this pull request as ready for review July 3, 2024 10:53

pavadeli self-assigned this Sep 10, 2024

pavadeli added 3 commits October 11, 2024 11:04

Merge branch 'main' into robin/picktype

0342943

Merge branch 'next' into robin/picktype

e88944b

fix merge issues

5e11e9b

pavadeli changed the base branch from main to next October 15, 2024 09:01

pavadeli added the on hold label Oct 16, 2024

pavadeli removed their assignment Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce `PickType` #92

feat: Introduce `PickType` #92

untio11 commented Jun 26, 2024 •

edited

Loading

pavadeli Jun 26, 2024

pavadeli Jun 26, 2024

pavadeli Jun 26, 2024

untio11 Jul 1, 2024

pavadeli Jun 26, 2024

untio11 Jul 3, 2024 •

edited

Loading

pavadeli Oct 15, 2024

untio11 Jul 3, 2024 •

edited

Loading

untio11 Jul 3, 2024

untio11 Jul 3, 2024

pavadeli commented Oct 15, 2024

feat: Introduce PickType #92

Are you sure you want to change the base?

feat: Introduce PickType #92

Conversation

untio11 commented Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untio11 Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untio11 Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavadeli commented Oct 15, 2024

feat: Introduce `PickType` #92

feat: Introduce `PickType` #92

untio11 commented Jun 26, 2024 •

edited

Loading

untio11 Jul 3, 2024 •

edited

Loading

untio11 Jul 3, 2024 •

edited

Loading