Add Dot Notation key support to utility functions #2256
Conversation
Types are complete, but needs to be tied into Document stuff later.
|
Oh, and before someone comments about use of |
| @@ -1,4 +1,11 @@ | |||
| import type { TypeOfTag } from "typescript/lib/typescript"; | |||
| import type { | |||
| DotNotationKeys, | |||
| DotNotationObject, | |||
There was a problem hiding this comment.
As GitHub Actions lints this import is unused.
| @@ -172,6 +179,7 @@ export declare function encodeURL(path: string): string; | |||
| * (default: `0`) | |||
| * @returns An expanded object | |||
| */ | |||
| export declare function expandObject<T extends Record<string, unknown>>(obj: FlatObject<T>, _d?: number): T; | |||
There was a problem hiding this comment.
The inference here isn't going to be ideal here, I think, based upon my prior experience with "inverse inference" -- where to infer T you have to take what obj is and apply the inverse of FlatObject which is obviously ExpandObject here. While on paper it's something the compiler could attempt to do, T is probably going to be inferred overly broad as like Record<string, unknown> or something. Here's some test cases you can add to try it out:
expectType<{ foo: { bar: 1 } }>(expandObject({ "foo.bar": 1 }));
expectType<{ foo: { bar: 1 } }>(expandObject({ "foo.bar": 1, "foo.lorem": 2 }));
expectType<{ foo: { bar: 1, lorem: 2 } }>(expandObject({ "foo": { "bar": 1 }, "foo.lorem": "2" }));We can get this draft in first and I can add in an ExpandObject helper type later if so desired as this is strictly an improvement.
| @@ -256,7 +265,7 @@ export function getType( | |||
| * @param key - An object property with notation a.b.c | |||
| * @returns An indicator for whether the property exists | |||
| */ | |||
| export declare function hasProperty(object: object, key: string): boolean; | |||
| export declare function hasProperty<T extends InputValue, key extends DotNotationKeys<T>>(object: T, key: key): boolean; | |||
There was a problem hiding this comment.
Nit; type parameters in this repo should be PascalCase so it should be Key extends DotNotationKeys<T>
| @@ -161,3 +161,95 @@ export type DataSourceForPlaceable<P extends PlaceableObject> = P extends Placea | |||
| ? D["_source"] | |||
| : never | |||
| : never; | |||
|
|
|||
| type EndValue = string | bigint | number | boolean | null | undefined; | |||
There was a problem hiding this comment.
I would call this PrimitiveValues as that seems to be a more descriptive name. Though if it's meant to be any primitive value it should be symbol as well.
| @@ -161,3 +161,95 @@ export type DataSourceForPlaceable<P extends PlaceableObject> = P extends Placea | |||
| ? D["_source"] | |||
| : never | |||
| : never; | |||
|
|
|||
| type EndValue = string | bigint | number | boolean | null | undefined; | |||
| export type InputValue = object | Array<unknown>; | |||
There was a problem hiding this comment.
object | Array<unknown> is redundant as object includes arrays. I'd do Record<string, unknown> | Array<unknown> for essentially clarity. Also I'd probably not make this exported.
| ? InnerGetValueFromDotKey<T[P[0] & keyof T], Extract<R, string[]>> | ||
| : never; | ||
|
|
||
| /** @internal exported for tests */ |
There was a problem hiding this comment.
I would say you should just test this by checking DotNotationObject's keys or something. If you're thoroughly testing DotNotationObject it shouldn't matter anyways.
| @@ -141,3 +141,5 @@ type PropertyTypeOrFallback<T, Key extends string, Fallback> = Key extends keyof | |||
| * Makes the given keys `K` of the type `T` required | |||
| */ | |||
| type RequiredProps<T, K extends keyof T> = Required<Pick<T, K>> & Omit<T, K>; | |||
|
|
|||
| type StringNumber<S extends string> = S extends `${number}` ? S : never; | |||
There was a problem hiding this comment.
I'd call this NumberToString, on the first usage I saw, I sort thought it was the inverse; a string to a number.
| expectType<{}>(foundry.utils.expandObject({})); | ||
| expectType<{ test: number }>(foundry.utils.expandObject({ test: 1 })); | ||
| expectType<{ test: number; deep: { value: number } }>( | ||
| foundry.utils.expandObject<{ test: number; deep: { value: number } }>({ |
There was a problem hiding this comment.
Honestly this test isn't really testing anything to do with expandObject itself as a function, it's testing that the first generic parameter is the return Ideally we wouldn't even make these types of generic parameters visible, it's just a way to access the parameters when getting the output. I would just remove it as this isn't a desired way to see it used. The inference will obviously be wrong but I can update this with an ExpandObject type.
| expectType<{ "0": number }>(foundry.utils.expandObject<{ "0": number }>({ "0": 1 })); | ||
|
|
||
| // hasProperty | ||
| expectType<boolean>(foundry.utils.hasProperty({ k1: 1 }, "k1")); |
There was a problem hiding this comment.
I would just note that hasProperty is fundamentally not as ideal as it could in Typescript. It can't really be expressed as a type guard without running into a million unions.
| declare function type<T>(): T; | ||
|
|
||
| // basic objects | ||
| expectType<"k1">(type<DotNotationKeys<{ k1: 1 }>>()); |
There was a problem hiding this comment.
While it's not a bad idea to start testing our types I think the type function helper here is clumsy.
Try this:
import { TypeEqual } from "ts-expect";
expectType<TypeEqual<DotNotationKeys{ k1: 1 }>, "k1">(true);It's still a a bit awkward but it's at least more expressive and something shown in tsd's examples.
Types are complete, but needs to be tied into Document stuff later. Hits complexity limits with the data model as is, and the v10 version is still being worked on.