-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add Dot Notation key support to utility functions #2256
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address Luke's comments.
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.