-
Notifications
You must be signed in to change notification settings - Fork 567
Fix metadata types #25736
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
Fix metadata types #25736
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,6 +105,42 @@ import { EmptyKey } from "../../../core/index.js"; | |
| type FromArray = TreeNodeFromImplicitAllowedTypes<[typeof Note, typeof Note]>; | ||
| type _check5 = requireTrue<areSafelyAssignable<FromArray, Note>>; | ||
| } | ||
| // TreeNodeFromImplicitAllowedTypes with a class | ||
| { | ||
| class NoteCustomized extends schema.object("Note", { text: schema.string }) { | ||
| public test: boolean = false; | ||
| } | ||
|
|
||
| type _check = requireAssignableTo<typeof NoteCustomized, TreeNodeSchema>; | ||
| type _checkNodeType = requireAssignableTo< | ||
| typeof NoteCustomized, | ||
| TreeNodeSchema<string, NodeKind, NoteCustomized> | ||
| >; | ||
|
|
||
| type Instance = InstanceType<typeof NoteCustomized>; | ||
| type _checkInstance = requireTrue<areSafelyAssignable<Instance, NoteCustomized>>; | ||
|
|
||
| type Test = TreeNodeFromImplicitAllowedTypes<typeof NoteCustomized>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: any reason this type isn't in-lined like the subsequent test cases?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I probable needed to mouse over it to investigate things at some point |
||
| type _check2 = requireTrue<areSafelyAssignable<Test, NoteCustomized>>; | ||
|
|
||
| type _check3 = requireTrue< | ||
| areSafelyAssignable< | ||
| TreeNodeFromImplicitAllowedTypes<[typeof NoteCustomized]>, | ||
| NoteCustomized | ||
| > | ||
| >; | ||
| type _check4 = requireTrue< | ||
| areSafelyAssignable< | ||
| TreeNodeFromImplicitAllowedTypes<[() => typeof NoteCustomized]>, | ||
| NoteCustomized | ||
| > | ||
| >; | ||
|
|
||
| type FromArray = TreeNodeFromImplicitAllowedTypes< | ||
| [typeof NoteCustomized, typeof NoteCustomized] | ||
| >; | ||
| type _check5 = requireTrue<areSafelyAssignable<FromArray, NoteCustomized>>; | ||
| } | ||
|
|
||
| // TreeFieldFromImplicitField | ||
| { | ||
|
|
@@ -383,29 +419,78 @@ describe("schemaFactory", () => { | |
| ); | ||
| }); | ||
|
|
||
| it("Node schema metadata", () => { | ||
| it("Node schema metadata - beta", () => { | ||
| const factory = new SchemaFactoryBeta(""); | ||
|
|
||
| const fooMetadata = { | ||
| description: "An object called Foo", | ||
| custom: { | ||
| baz: true, | ||
| }, | ||
| }; | ||
| } as const; | ||
|
|
||
| class Foo extends factory.object( | ||
| "Foo", | ||
| { bar: factory.number }, | ||
| { metadata: fooMetadata }, | ||
| ) {} | ||
|
|
||
| // Ensure `Foo.metadata` is typed as we expect, and we can access its fields without casting. | ||
| const description = Foo.metadata.description; | ||
| type _check1 = requireTrue<areSafelyAssignable<typeof description, string | undefined>>; | ||
|
|
||
| const custom = Foo.metadata.custom; | ||
|
|
||
| // Currently it is impossible to have required custom metadata: it always includes undefined as an option. | ||
| // TODO: having a way to make metadata required would be nice. | ||
|
|
||
| type _check2 = requireTrue< | ||
| areSafelyAssignable<typeof custom, { baz: true } | undefined> | ||
| >; | ||
| assert(custom !== undefined); | ||
|
|
||
| const baz = custom.baz; | ||
| type _check3 = requireTrue<areSafelyAssignable<typeof baz, true>>; | ||
|
|
||
| // This must be checked after the types are checks to avoid it narrowing and making the type checks above not test anything. | ||
|
||
| assert.deepEqual(Foo.metadata, fooMetadata); | ||
| }); | ||
|
|
||
| it("Node schema metadata - alpha", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily for this PR, but should we be running most of these tests over all of the applicable variants of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There isn't really a good way to do that since we are testing typing so looping over them wouldn't work so we would have to write out multiple tests like I did here. I only added them because there was actually a bug, but yes this is something we might want to do when the typing for the overloads is different and complex. Hopefulyl we can get away from having logic duplicated at multiple layers to reduce risk here. |
||
| const factory = new SchemaFactoryAlpha(""); | ||
|
|
||
| const fooMetadata = { | ||
| description: "An object called Foo", | ||
| custom: { | ||
| baz: true, | ||
| }, | ||
| } as const; | ||
|
|
||
| class Foo extends factory.objectAlpha( | ||
| "Foo", | ||
| { bar: factory.number }, | ||
| { metadata: fooMetadata }, | ||
| ) {} | ||
|
|
||
| // Ensure `Foo.metadata` is typed as we expect, and we can access its fields without casting. | ||
| const description = Foo.metadata.description; | ||
| const baz = Foo.metadata.custom.baz; | ||
| type _check1 = requireTrue<areSafelyAssignable<typeof description, string>>; | ||
| type _check2 = requireTrue<areSafelyAssignable<typeof baz, boolean>>; | ||
| type _check1 = requireTrue<areSafelyAssignable<typeof description, string | undefined>>; | ||
|
|
||
| const custom = Foo.metadata.custom; | ||
|
|
||
| // Currently it is impossible to have required custom metadata: it always includes undefined as an option. | ||
| // TODO: having a way to make metadata required would be nice. | ||
|
|
||
| type _check2 = requireTrue< | ||
| areSafelyAssignable<typeof custom, { baz: true } | undefined> | ||
| >; | ||
| assert(custom !== undefined); | ||
|
|
||
| const baz = custom.baz; | ||
| type _check3 = requireTrue<areSafelyAssignable<typeof baz, true>>; | ||
|
|
||
| // This must be checked after the types are checks to avoid it narrowing and making the type checks above not test anything. | ||
CraigMacomber marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| assert.deepEqual(Foo.metadata, fooMetadata); | ||
| }); | ||
|
|
||
| it("Field schema metadata", () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -28,6 +28,7 @@ import { | |||||
| type AllowedTypesFull, | ||||||
| type ImplicitAllowedTypes, | ||||||
| type AllowedTypes, | ||||||
| SchemaFactoryBeta, | ||||||
| } from "../../../simple-tree/index.js"; | ||||||
| import { | ||||||
| allowUnused, | ||||||
|
|
@@ -498,8 +499,8 @@ describe("SchemaFactory Recursive methods", () => { | |||||
| } | ||||||
| }); | ||||||
|
|
||||||
| it("Node schema metadata", () => { | ||||||
| const factory = new SchemaFactoryAlpha(""); | ||||||
| it("Node schema metadata - beta", () => { | ||||||
| const factory = new SchemaFactoryBeta(""); | ||||||
| class Foo extends factory.objectRecursive( | ||||||
| "Foo", | ||||||
| { bar: [() => Foo] }, | ||||||
|
|
@@ -511,14 +512,59 @@ describe("SchemaFactory Recursive methods", () => { | |||||
| }, | ||||||
| ) {} | ||||||
|
|
||||||
| const custom = Foo.metadata.custom; | ||||||
|
|
||||||
| // Currently it is impossible to have required custom metadata: it always includes undefined as an option. | ||||||
| // TODO: having a way to make metadata required would be nice. | ||||||
| type _check1 = requireTrue< | ||||||
| areSafelyAssignable<typeof custom, { baz: true } | undefined> | ||||||
| >; | ||||||
| assert(custom !== undefined); | ||||||
|
|
||||||
| // Ensure `Foo.metadata` is typed as we expect, and we can access its fields without casting. | ||||||
| const baz = custom.baz; | ||||||
|
|
||||||
| type _check2 = requireTrue<areSafelyAssignable<typeof baz, true>>; | ||||||
|
|
||||||
| // This must be checked after the types are checks to avoid it narrowing and making the type checks above not test anything. | ||||||
|
||||||
| // This must be checked after the types are checks to avoid it narrowing and making the type checks above not test anything. | |
| // This must be checked after the types are checked to avoid it narrowing and making the type checks above not test anything. |
Uh oh!
There was an error while loading. Please reload this page.