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 2 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
1 change: 1 addition & 0 deletions src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ export * from './record';
export * from './string';
export * from './union';
export * from './unknown';
export * from './pick';
pavadeli marked this conversation as resolved.
Show resolved Hide resolved
219 changes: 219 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,219 @@
import { The } from '../interfaces';
import { createExample, testTypeImpl } from '../testutils';
import { boolean } from './boolean';
import { object, partial } from './interface';
import { number } from './number';
import { pick, pickProperties, pickPropertiesInfo } from './pick';
import { string } from './string';
import './intersection';
pavadeli marked this conversation as resolved.
Show resolved Hide resolved
import { intersection } from './intersection';
import { union } from './union.js';
import { literal } from './literal.js';
import { array } from './array.js';
pavadeli marked this conversation as resolved.
Show resolved Hide resolved

type TestType = The<typeof TestType>;
const TestType = object('TestType', {
a: string,
b: boolean,
c: number,
});

testTypeImpl({
name: `Pick<TestType, 'b' | 'c'>`,
type: pick(TestType, ['b', 'c']),
basicType: 'object',
validValues: [
{ b: true, c: 5 },
{ b: false, c: -2 },
],
invalidValues: [
[{ b: 'blah', c: 3 }, `error in [Pick<TestType, 'b' | 'c'>] at <b>: expected a boolean, got a string ("blah")`],
[{ c: 3 }, `error in [Pick<TestType, 'b' | 'c'>]: missing property <b> [boolean], got: { c: 3 }`],
],
validConversions: [
[
{ a: 'drop', b: true, c: 2 },
{ b: true, c: 2 },
],
],
});

type PartialTestType = The<typeof PartialTestType>;
const PartialTestType = partial('PartialTestType', {
d: string,
e: number,
});
testTypeImpl({
name: `Pick<PartialTestType, 'd'>`,
type: pick(PartialTestType, ['d']),
basicType: 'object',
validValues: [{ d: 'blah' }, {}],
invalidValues: [[{ d: undefined }, `error in [Pick<PartialTestType, 'd'>] at <d>: expected a string, got an undefined`]],
validConversions: [
[{ b: true, c: 3 }, {}],
[{ d: 'hns', e: 1 }, { d: 'hns' }],
pavadeli marked this conversation as resolved.
Show resolved Hide resolved
],
});

type IntersectionTestType = The<typeof IntersectionTestType>;
const IntersectionTestType = intersection('IntersectionTestType', [TestType, PartialTestType]);
testTypeImpl({
name: `Pick<IntersectionTestType, 'a' | 'd'>`,
type: pick(IntersectionTestType, ['a', 'd']),
basicType: 'object',
validValues: [{ a: 'blah', d: 'dah' }, { a: 'blah2' }, { a: 'hts', d: '' }],
invalidValues: [
[{ d: 'dah' }, `error in [Pick<IntersectionTestType, 'a' | 'd'>]: missing property <a> [string], got: { d: "dah" }`],
[{ a: 'blah2', d: undefined }, `error in [Pick<IntersectionTestType, 'a' | 'd'>] at <d>: expected a string, got an undefined`],
],
validConversions: [
[
{ a: 'drop', b: true, c: 2, d: 'tnsh', e: 2 },
Copy link
Member

Choose a reason for hiding this comment

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

Same as before, why not use invalid values for some of the omitted properties?

{ a: 'drop', d: 'tnsh' },
],
[{ a: 'tns' }, { a: 'tns' }],
Copy link
Member

Choose a reason for hiding this comment

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

This is already checked as part of the validValues.

],
});

type Overlap = The<typeof Overlap>;
const Overlap = object('Overlap', {
a: string,
b: boolean,
d: boolean,
});
type UnionTestType = The<typeof UnionTestType>;
const UnionTestType = union('UnionTestType', [TestType, Overlap]);
// const pickunion = pick(UnionTestType, ['a']);
testTypeImpl({
name: `Pick<UnionTestType, 'a'>`,
type: pick(UnionTestType, ['a']),
basicType: 'object',
validValues: [{ a: 'str' }, { a: '' }],
invalidValues: [
[{}, `error in [Pick<UnionTestType, 'a'>]: missing property <a> [string | string], got: {}`],
[{ a: true }, `error in [Pick<UnionTestType, 'a'>] at <a>: expected a string, got a boolean (true)`],
],
validConversions: [
[{ a: 'Overlap', b: true }, { a: 'Overlap' }],
[{ a: 'TestType', b: false, c: 12 }, { a: 'TestType' }],
],
});
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this is only one of the interesting cases with unions (overlap with the same property type) there are other that need to be tested (and some do not work as expected yet):

  • Pick<{ a: string, b: boolean} | { a: string, b: boolean }, 'a'>
  • Pick<{ a: string } | { a: boolean }, 'a'> should accept both a string and a boolean for a
  • But even more interesting, a discriminated union:
    Pick<
        | { t: 'string', v: string, obsolete: string }
        | { t: 'number', v: number, otherStuffBeGone: unknown },
        't' | 'v'
    >
    Here we have a choice, but I believe we should distribute over the elements of the union, see this playground link for more info.
  • And what should happen in this case?
    Pick<
        | { t: 'string', v: string, obsolete: string }
        | { t: 'number', v: number, otherStuffBeGone: unknown }
        | { t: 'none' }, // this variant has no `v` property, are we allowed to pick 'v'?
        't' | 'v'
    >
    See this playground link


type NastyType = The<typeof NastyType>;
const NastyType = object('NastyType', {
union: UnionTestType,
intersection: pick(IntersectionTestType, ['a', 'd']),
inlineUnion: literal('stringliteral').or(literal(12)),
nestedOptionals: object({
req: string,
}).withOptional({
opt: string,
}),
arr: array(number).withConstraint('nonempty', i => i.length > 0),
}).withOptional({
optionalUnion: pick(UnionTestType, ['a']).or(pick(IntersectionTestType, ['c', 'd'])),
optionalIntersection: pick(IntersectionTestType, ['a', 'd']),
});
testTypeImpl({
type: pick(NastyType, ['inlineUnion', 'optionalIntersection']),
name: `Pick<NastyType, 'inlineUnion' | 'optionalIntersection'>`,
basicType: 'object',
validValues: [
{ inlineUnion: 'stringliteral', optionalIntersection: { a: 'reqstring', d: 'optionalstring' } },
{ inlineUnion: 12, optionalIntersection: { a: 'reqstring' } },
{ inlineUnion: 'stringliteral' },
],
invalidValues: [
[
{ inlineUnion: 13, optionalIntersection: { a: 'reqstring', d: 'optionalstring' } },
[
`error in [Pick<NastyType, 'inlineUnion' | 'optionalIntersection'>] at <inlineUnion>: in union element [12]: expected the literal 12, got: 13`,
` • disregarded 1 union-subtype that does not accept a number`,
],
],
[
{ optionalIntersection: { a: 'reqstring', d: 'optionalstring' } },
`error in [Pick<NastyType, 'inlineUnion' | 'optionalIntersection'>]: missing property <inlineUnion> ["stringliteral" | 12], got: { optionalIntersection: { a: "reqstring", d: " .. " } }`,
],
[
{ inlineUnion: 12, optionalIntersection: { a: 'reqstring', d: undefined } },
`error in [Pick<NastyType, 'inlineUnion' | 'optionalIntersection'>] at <optionalIntersection.d>: expected a string, got an undefined`,
],
[
{ inlineUnion: 12, optionalIntersection: {} },
`error in [Pick<NastyType, 'inlineUnion' | 'optionalIntersection'>] at <optionalIntersection>: missing property <a> [string], got: {}`,
],
],
});

const example1 = createExample(NastyType, 1);
const example2 = createExample(NastyType, 2);

testTypeImpl({
type: pick(NastyType, ['optionalUnion', 'arr']),
name: `Pick<NastyType, 'optionalUnion' | 'arr'>`,
basicType: 'object',
validValues: [
{ optionalUnion: { a: 'pickedUnion' }, arr: [1] },
{ optionalUnion: { c: 12, d: 'pickedIntersection' }, arr: [12, 3] },
{ optionalUnion: { c: 1 }, arr: [12, 3] },
{ arr: [12, 3] },
],
invalidValues: [
[
{ optionalUnion: 13, arr: [] },
[
`errors in [Pick<NastyType, 'optionalUnion' | 'arr'>]:`,
`- at <optionalUnion>: expected an object, got a number (13)`,
`- at <arr>: expected a [nonempty], got: []`,
Copy link
Member

Choose a reason for hiding this comment

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

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

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: []`,

],
],
[
{ optionalUnion: { a: 'from UnionTestType', c: 'from IntersectionTestType', d: 'from IntersectionTestType' }, arr: [12] },
`error`,
],
],
validConversions: [[example1, { optionalUnion: example1.optionalUnion, arr: example1.arr }]],
});

describe('Other', () => {
test('narrowing', () => {
expect(pick(NastyType, ['optionalUnion', 'arr'])(example1)).toEqual({
optionalUnion: {
a: 'xxxxxxxxxxxxxxxx',
},
arr: example1.arr,
});
expect(pick(NastyType, ['optionalUnion', 'arr'])(example2)).toEqual({
optionalUnion: {
c: 0.14,
d: 'xxxxxxxxxxxxxxx',
},
arr: example2.arr,
});
});
test('pickProperties', () => {
expect(pickProperties(TestType.props, ['a', 'b'])).toEqual({
a: string,
b: boolean,
});
});
test('pickPropertiesInfo', () => {
expect(pickPropertiesInfo(TestType.propsInfo, ['a', 'b'])).toEqual({
a: {
partial: false,
type: string,
},
b: {
partial: false,
type: boolean,
},
});
expect(pickPropertiesInfo(PartialTestType.propsInfo, ['d'])).toEqual({
d: {
partial: true,
type: string,
},
});
});
});
103 changes: 103 additions & 0 deletions src/types/pick.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import { BaseObjectLikeTypeImpl, createType } from '../base-type.js';
import type {
BasicType,
MessageDetails,
OneOrMore,
PossibleDiscriminator,
Properties,
PropertiesInfo,
Result,
Type,
TypeImpl,
TypeOf,
ValidationOptions,
Visitor,
} from '../interfaces.js';
import { decodeOptionalName } from '../utils/collection-utils.js';
import { define } from '../utils/define.js';
import { unknownRecord } from './unknown.js';
import { hasOwnProperty } from '../utils/type-utils.js';
import { prependPathToDetails } from '../utils/failure-utils.js';
import { interfaceStringify } from '../utils/stringifiers.js';
import { propsInfoToProps } from './union.js';
pavadeli marked this conversation as resolved.
Show resolved Hide resolved

export class PickType<Type extends BaseObjectLikeTypeImpl<unknown>, ResultType> extends BaseObjectLikeTypeImpl<ResultType> {
readonly name: string;
readonly isDefaultName: boolean;
readonly props: Properties;
override propsInfo: PropertiesInfo;
constructor(
readonly type: Type,
readonly keys: OneOrMore<keyof TypeOf<Type>>,
name?: string,
) {
super();
this.isDefaultName = !name;
this.name = name || `Pick<${type.name}, ${keys.map(k => `'${k.toString()}'`).join(' | ')}>`;
const stringKeys = this.keys.map(k => k.toString()) as [string, ...string[]];
this.propsInfo = pickPropertiesInfo(this.type.propsInfo, stringKeys);
this.props = propsInfoToProps(this.propsInfo);
this.possibleDiscriminators = this.type.possibleDiscriminators;
}
override possibleDiscriminators: readonly PossibleDiscriminator[] = this.type.possibleDiscriminators; // TODO: Filter
override basicType!: BasicType;
override typeConfig: unknown;

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 {
// TODO: Should be fine, right?
Copy link
Member

Choose a reason for hiding this comment

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

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

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<Type extends BaseObjectLikeTypeImpl<unknown>, Keys extends OneOrMore<keyof TypeOf<Type>>>(
...args: [name: string, baseType: Type, keys: Keys] | [baseType: Type, keys: Keys]
): TypeImpl<PickType<Type, Pick<TypeOf<Type>, Keys[number]>>> {
const [name, baseType, keys] = decodeOptionalName(args);
return createType(new PickType(baseType, keys, name));
}

export function pickProperties(props: Properties, keys: OneOrMore<keyof Properties>) {
const properties = keys
.map(property => [property, props[property]] as const)
.filter<[string, Type<any>]>((v): v is [string, Type<any>] => v[1] !== undefined);
return Object.fromEntries(properties);
}

export function pickPropertiesInfo(propsInfo: PropertiesInfo, keys: OneOrMore<keyof Properties>) {
const properties = keys
.map(property => [property, propsInfo[property]] as const)
.filter<[string, PropertiesInfo[keyof PropertiesInfo]]>(
(v): v is [string, PropertiesInfo[keyof PropertiesInfo]] => v[1] !== undefined,
);
return Object.fromEntries(properties);
}
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