Skip to content
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.

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

feat: Introduce PickType #92

Open
wants to merge 7 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
5 changes: 3 additions & 2 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
"editor.detectIndentation": false,
"editor.formatOnSave": true,
"editor.codeActionsOnSave": {
"source.organizeImports": "explicit",
"source.fixAll": "explicit"
},
"typescript.tsdk": "node_modules/typescript/lib",
"typescript.enablePromptUseWorkspaceTsdk": true,
"files.associations": { "*.json": "jsonc" },
"javascript.preferences.importModuleSpecifierEnding": "js",
"typescript.preferences.importModuleSpecifierEnding": "js",
"javascript.preferences.importModuleSpecifierEnding": "minimal",
"typescript.preferences.importModuleSpecifierEnding": "minimal",
"files.readonlyInclude": {
"etc/**/*": true,
"markdown/**/*": true,
Expand Down
2 changes: 1 addition & 1 deletion src/base-type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ export abstract class BaseTypeImpl<ResultType, TypeConfig = unknown> implements
* See {@link UnionType} for more information about unions.
*/
// istanbul ignore next: using ordinary stub instead of module augmentation to lighten the load on the TypeScript compiler
or<Other extends BaseTypeImpl<unknown>>(_other: Other): Type<ResultType | TypeOf<Other>> {
or<Other extends BaseTypeImpl<unknown>>(_other: Other): ObjectType<ResultType | TypeOf<Other>> {
throw new Error('stub');
}

Expand Down
1 change: 1 addition & 0 deletions src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export * from './intersection';
export * from './keyof';
export * from './literal';
export * from './number';
export * from './pick';
export * from './record';
export * from './string';
export * from './union';
Expand Down
196 changes: 196 additions & 0 deletions src/types/pick.test.ts
pavadeli marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
import { The } from '../interfaces';
import { assignableTo, testTypeImpl } from '../testutils';
import { checkOneOrMore } from '../utils';
import { array } from './array';
import { boolean } from './boolean';
import { object, partial } from './interface';
import { intersection } from './intersection';
import { literal } from './literal';
import { number } from './number';
import { narrowPickedKeys, pick, validPick } from './pick';
import { string } from './string';
import { union } from './union';

type A = The<typeof A>;
const A = object('A', {
a: string,
b: boolean,
c: number,
});
testTypeImpl({
type: pick('MyCustomName', A, ['a', 'b']),
name: 'MyCustomName',
});
testTypeImpl({
type: pick(A, ['b', 'c']),
name: `Pick<A, 'b' | 'c'>`,
basicType: 'object',
validValues: [
{ b: true, c: 5 },
{ b: false, c: -2 },
],
invalidValues: [
[{ b: 'hi', c: 3 }, `error in [Pick<A, 'b' | 'c'>] at <b>: expected a boolean, got a string ("hi")`],
[{ c: 3 }, `error in [Pick<A, 'b' | 'c'>]: missing property <b> [boolean], got: { c: 3 }`],
],
validConversions: [
[
{ a: 42, b: true, c: 2 }, // `a` is actually of the wrong type, but that shouldn't matter.
{ b: true, c: 2 },
],
],
});

/** No overlap in property names with `A`. */
type B = The<typeof B>;
const B = partial('B', {
d: string,
e: number,
});
testTypeImpl({
name: `Pick<B, 'd'>`,
type: pick(B, ['d']),
basicType: 'object',
validValues: [{ d: 'string' }, {}],
invalidValues: [[{ d: undefined }, `error in [Pick<B, 'd'>] at <d>: expected a string, got an undefined`]],
validConversions: [
[{ b: true, c: 3 }, {}], // Property `b` and `c` don't even exist on `B`.
[{ d: 'string value', e: 'no validation here' }, { d: 'string value' }],
],
});

type IntersectAB = The<typeof IntersectAB>;
const IntersectAB = intersection('IntersectAB', [A, B]);
testTypeImpl({
name: `Pick<IntersectAB, 'a' | 'd'>`,
type: pick(IntersectAB, ['a', 'd']),
basicType: 'object',
validValues: [{ a: 'string a', d: 'string d' }, { a: 'string a' }, { a: 'string a', d: '' }],
invalidValues: [
[{ d: 'string d' }, `error in [Pick<IntersectAB, 'a' | 'd'>]: missing property <a> [string], got: { d: "string d" }`],
[{ a: 'string a', d: undefined }, `error in [Pick<IntersectAB, 'a' | 'd'>] at <d>: expected a string, got an undefined`],
],
validConversions: [
[
{ a: 'string a', b: 'not a bool', c: 2, d: 'string d', e: false },
{ a: 'string a', d: 'string d' },
],
],
});

/** Property `a` and `b` overlapping in name and type with A, but `d` doesn't exist in A. */
type C = The<typeof C>;
const C = object('C', {
a: string,
b: number,
d: boolean,
});
type PickedUnionAC = The<typeof PickedUnionAC>;
const PickedUnionAC = pick(union('UnionAC', [A, C]), ['a', 'd']);
testTypeImpl({
name: `Pick<UnionAC, 'a' | 'd'>`,
type: PickedUnionAC,
basicType: 'object',
validValues: [{ a: 'string a', d: true }, { a: 'string a' }],
invalidValues: [
[
{ d: true },
[
`error in [Pick<UnionAC, 'a' | 'd'>]: failed every element in union:`,
`(got: { d: true })`,
` • error in [Pick<C, 'a' | 'd'>]: missing property <a> [string]`,
` • error in [Pick<A, 'a'>]: missing property <a> [string]`,
],
],
],
validConversions: [
[{ a: 'string a', b: true }, { a: 'string a' }],
[
{ a: 'string a2', d: true, c: undefined },
{ a: 'string a2', d: true },
],
],
});
assignableTo<PickedUnionAC>({ a: 'string' });
assignableTo<PickedUnionAC>({ a: 'string', d: true });
// @ts-expect-error because no union element accepts just { d: boolean }
assignableTo<PickedUnionAC>({ d: true });

/** { a: string; b: boolean; }
* | { a: string; b: number; }
* | { a: number; b: string; } */
type DiscriminatedUnion = The<typeof DiscriminatedUnion>;
const DiscriminatedUnion = pick('DiscriminatedUnion', A.or(C).or(object({ a: number, b: string, c: number })), ['a', 'b']);
testTypeImpl({
name: `DiscriminatedUnion`,
type: DiscriminatedUnion,
basicType: 'object',
validValues: [
{ a: 'string', b: true },
{ a: 'string', b: 2 },
{ a: 3, b: 'string' },
],
invalidConversions: [
[
{ a: 'string 1', b: 'string 2', c: null },
[
`error in [DiscriminatedUnion]: failed every element in union:`,
`(got: { a: "string 1", b: "string 2", c: null })`,
` • error in [Pick<A | C, 'a' | 'b'>] at <b>: expected a boolean or a number, got a string ("string 2")`,
` • error in [Pick<{ a: number, b: string, c: number }, 'a' | 'b'>] at <a>: expected a number, got a string ("string 1")`,
],
],
],
});
assignableTo<DiscriminatedUnion>({ a: 'string', b: true });
assignableTo<DiscriminatedUnion>({ a: 'string', b: 3 });
assignableTo<DiscriminatedUnion>({ a: 3, b: 'true' });
// @ts-expect-error because no union element accepts string, string
assignableTo<DiscriminatedUnion>({ a: 'string', b: 'true' });
// @ts-expect-error because no union element accepts number, bool
assignableTo<DiscriminatedUnion>({ a: 3, b: true });
// @ts-expect-error because no union element accepts number, number
assignableTo<DiscriminatedUnion>({ a: 3, b: 3 });

// { a: 'string' } | { b: true }, picking 'b' => { b: true } is the only valid variant for the picked key.
type PickUniqueVariantProp = The<typeof PickUniqueVariantProp>;
const PickUniqueVariantProp = pick(object({ a: literal('string') }).or(object({ b: literal(true) })), ['b']);
testTypeImpl({
name: `Pick<{ a: "string" } | { b: true }, 'b'>`,
type: PickUniqueVariantProp,
basicType: 'object',
validValues: [{ b: true }],
validConversions: [[{ a: 'string', b: true, c: null }, { b: true }]],
});

describe('Other', () => {
test('Invalid basic types for input', () => {
// When parsing input on a valid pick type
expect(() => DiscriminatedUnion(1)).toThrow('error in [DiscriminatedUnion]: expected an object, got a number');
// When constructing a new pick type from a union
expect(() => pick(array(A).or(B), ['d'])).toThrow(`Can only pick elements of unions with 'object' as basic type.`);
});
describe('Union variant validation helper functions', () => {
const types = checkOneOrMore([
object({ a: literal('a'), b: literal('b'), c: literal('c') }),
object({ b: literal('b'), c: literal('c'), d: literal('d') }),
object({ c: literal('c'), d: literal('d'), e: literal('e') }),
]);
test.each`
pickedKeys | narrowedKeys | isValidPick
${['a']} | ${[['a'], [], []]} | ${true}
${['b']} | ${[['b'], ['b'], []]} | ${true}
${['c']} | ${[['c'], ['c'], ['c']]} | ${true}
${['a', 'b']} | ${[['a', 'b'], ['b'], []]} | ${true}
${['c', 'd']} | ${[['c'], ['c', 'd'], ['c', 'd']]} | ${true}
${['a', 'd']} | ${[['a'], ['d'], ['d']]} | ${false}
${['a', 'e']} | ${[['a'], [], ['e']]} | ${false}
`('Picking $pickedKeys should be valid: $isValidPick', ({ pickedKeys, narrowedKeys, isValidPick }) => {
expect(narrowPickedKeys(pickedKeys, types)).toEqual(narrowedKeys);
expect(validPick(pickedKeys, narrowedKeys)).toBe(isValidPick);
});
test('Impossible union variant pick construction should throw', () => {
expect(() => pick('Unpickable', union(types), ['a', 'd'])).toThrow('Selected keys describe impossible union variant.');
});
});
});
139 changes: 139 additions & 0 deletions src/types/pick.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
import { BaseObjectLikeTypeImpl, createType } from '../base-type';
import type {
BasicType,
MessageDetails,
ObjectType,
OneOrMore,
PossibleDiscriminator,
Properties,
PropertiesInfo,
Result,
TypeOf,
ValidationOptions,
Visitor,
} from '../interfaces';
import { checkOneOrMore, decodeOptionalName, define, hasOwnProperty, interfaceStringify, prependPathToDetails } from '../utils';
import { UnionType, propsInfoToProps, union } from './union';
import { unknownRecord } from './unknown';

type PickableImpl = BaseObjectLikeTypeImpl<unknown>;
type PickableKeys<Type> = Type extends unknown ? keyof Type & string : never;
type DistributedPick<Type, Keys extends PickableKeys<Type>> = Type extends unknown ? Pick<Type, Keys & keyof Type> : never;

export class PickType<Type extends PickableImpl, ResultType> extends BaseObjectLikeTypeImpl<ResultType> {
readonly name: string;
readonly isDefaultName: boolean;
readonly props: Properties;
override propsInfo: PropertiesInfo = pickPropertiesInfo(this.type.propsInfo, this.keys);
override basicType!: BasicType;
override typeConfig: unknown;
override possibleDiscriminators: readonly PossibleDiscriminator[] = this.type.possibleDiscriminators.filter(disc =>
(this.keys as (string | undefined)[]).includes(disc.path[0]),
);

constructor(
readonly type: Type,
readonly keys: OneOrMore<PickableKeys<TypeOf<Type>>>,
name?: string,
) {
super();
this.isDefaultName = !name;
this.name = name || `Pick<${type.name}, ${keys.map(k => `'${k}'`).join(' | ')}>`;
this.props = propsInfoToProps(this.propsInfo);
}

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);
}
Copy link
Member

Choose a reason for hiding this comment

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

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?1
  • 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

  1. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nieuwe issue voor aangemaakt #93

override accept<R>(visitor: Visitor<R>): R {
return visitor.visitObjectLikeType(this);
}
/** {@inheritdoc BaseTypeImpl.maybeStringify} */
override maybeStringify(value: ResultType): string {
return interfaceStringify(this.propsArray, value as Record<string, unknown>);
}
}
define(PickType, 'basicType', 'object');

export function pick<TypeImpl extends PickableImpl, Keys extends PickableKeys<TypeOf<TypeImpl>>>(
...args: [name: string, baseType: TypeImpl, keys: OneOrMore<Keys>] | [baseType: TypeImpl, keys: OneOrMore<Keys>]
): ObjectType<DistributedPick<TypeOf<TypeImpl>, Keys>> {
const [name, baseType, keys] = decodeOptionalName(args);
if (baseType instanceof UnionType) {
const unionType: UnionType<OneOrMore<BaseObjectLikeTypeImpl<TypeOf<TypeImpl>>>, TypeOf<TypeImpl>> = baseType;
if (!(unionType.basicType === 'object')) {
throw new Error(`Can only pick elements of unions with 'object' as basic type.`);
}
const narrowedKeys = narrowPickedKeys(keys, unionType.types);
if (!validPick(keys, narrowedKeys)) {
throw new Error('Selected keys describe impossible union variant.');
}

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 ?? []);
Comment on lines +93 to +104
Copy link
Contributor Author

@untio11 untio11 Jul 3, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


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),
Comment on lines +106 to +109
Copy link
Contributor Author

@untio11 untio11 Jul 3, 2024

Choose a reason for hiding this comment

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

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.

) as unknown as ObjectType<DistributedPick<TypeOf<TypeImpl>, Keys>>;
}

return createType(new PickType(baseType, keys, name));
}

export const pickPropertiesInfo = (propsInfo: PropertiesInfo, keys: OneOrMore<keyof Properties>) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Object.fromEntries(
keys
.map(property => [property, propsInfo[property]])
.filter((v): v is [string, PropertiesInfo[keyof PropertiesInfo]] => v[1] !== undefined),
);

/** Return the intersection of picked keys with each of the property keys of the inner types. */
export const narrowPickedKeys = <TypeImpl extends PickableImpl, Keys extends PickableKeys<TypeOf<TypeImpl>>>(
pickedKeys: OneOrMore<Keys>,
innerTypes: OneOrMore<TypeImpl>,
): OneOrMore<Keys[]> =>
innerTypes
.map(type => Object.keys(type.props))
.map(innerKeys => pickedKeys.filter(pickedKey => innerKeys.find(innerKey => pickedKey === innerKey))) as OneOrMore<Keys[]>;

/**
* Check if the intersection of all the non-empty narrowed keys is not empty. If it is, it means the picked keys describe an impossible
* union variant. Empty narrowed keys are ignored, because they will not end up in the union at all.
*/
export const validPick = (pickedKeys: string[], innerTypeKeys: OneOrMore<string[]>): boolean =>
innerTypeKeys.reduce((intersect, keys) => intersect.filter(k1 => (keys.length ? keys.find(k2 => k1 === k2) : true)), pickedKeys)
.length > 0;
2 changes: 1 addition & 1 deletion src/types/union.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ function analyzePossibleDiscriminators(
return found ? [...found.values()] : [];
}

function propsInfoToProps(propsInfo: PropertiesInfo): Properties {
export function propsInfoToProps(propsInfo: PropertiesInfo): Properties {
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 one I'm using in pick now, but also don't know if it's okay to export just like that.

const result: Properties = {};
for (const [key, { type }] of Object.entries(propsInfo)) {
result[key] = type;
Expand Down
Loading