-
Notifications
You must be signed in to change notification settings - Fork 532
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
Ai collab explicit #22836
base: main
Are you sure you want to change the base?
Ai collab explicit #22836
Changes from 24 commits
5f912d8
ab3b543
822092e
478c5b2
4b5bc91
c8b6684
3466402
62b3783
ecd9746
ccfe5b6
2be8309
a01a52a
a29d48a
0cf8cfe
232d335
e5e3b2d
b60ecf3
c3c50d8
1cd8d8b
3c1934c
6c3b76d
f3bd57b
b0e2984
c9d79b5
8f1f36f
bac3094
6eb6623
7bfba91
a79e33c
b675572
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 |
---|---|---|
|
@@ -159,6 +159,23 @@ export { | |
getJsonSchema, | ||
type LazyItem, | ||
type Unenforced, | ||
type SimpleNodeSchemaBase, | ||
type SimpleTreeSchema, | ||
type SimpleNodeSchema, | ||
type SimpleFieldSchema, | ||
type SimpleLeafNodeSchema, | ||
type SimpleMapNodeSchema, | ||
type SimpleArrayNodeSchema, | ||
type SimpleObjectNodeSchema, | ||
normalizeFieldSchema, | ||
isTreeNodeSchemaClass, | ||
normalizeAllowedTypes, | ||
getSimpleSchema, | ||
numberSchema, | ||
stringSchema, | ||
booleanSchema, | ||
handleSchema, | ||
nullSchema, | ||
Comment on lines
+174
to
+178
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. Are these needed? Can we not use the |
||
type ReadonlyArrayNode, | ||
} from "./simple-tree/index.js"; | ||
export { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,6 +137,10 @@ function convertObjectNodeSchema(schema: SimpleObjectNodeSchema): JsonObjectNode | |
const properties: Record<string, JsonFieldSchema> = {}; | ||
const required: string[] = []; | ||
for (const [key, value] of Object.entries(schema.fields)) { | ||
if (value.metadata?.omitFromJson === true) { | ||
// Don't emit JSON Schema for fields which specify they should be excluded. | ||
continue; | ||
} | ||
const allowedTypes: JsonSchemaRef[] = []; | ||
for (const allowedType of value.allowedTypes) { | ||
allowedTypes.push(createSchemaRef(allowedType)); | ||
|
@@ -150,8 +154,8 @@ function convertObjectNodeSchema(schema: SimpleObjectNodeSchema): JsonObjectNode | |
}; | ||
|
||
// Don't include "description" property at all if it's not present in the input. | ||
if (value.description !== undefined) { | ||
output.description = value.description; | ||
if (value.metadata?.description !== undefined) { | ||
output.description = value.metadata.description; | ||
Comment on lines
+153
to
+154
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. We're deliberately not doing anything with the rest of the metadata here, right? 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. The global metadata bag isn't necessarily JSON compatible. The |
||
} | ||
|
||
properties[key] = output; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -172,12 +172,21 @@ describe("simpleSchemaToJsonSchema", () => { | |
"foo": { | ||
kind: FieldKind.Optional, | ||
allowedTypes: new Set<string>(["test.number"]), | ||
description: "A number representing the concept of Foo.", | ||
metadata: { description: "A number representing the concept of Foo." }, | ||
}, | ||
"bar": { | ||
kind: FieldKind.Required, | ||
allowedTypes: new Set<string>(["test.string"]), | ||
description: "A string representing the concept of Bar.", | ||
metadata: { description: "A string representing the concept of Bar." }, | ||
}, | ||
"id": { | ||
kind: FieldKind.Identifier, | ||
allowedTypes: new Set<string>(["test.string"]), | ||
metadata: { | ||
description: "Unique identifier for the test object.", | ||
// IDs should be generated by the system. Hide from the JSON Schema. | ||
omitFromJson: true, | ||
}, | ||
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 100% sure but this (the comment in particular) feels specific to the ai-collab example app use case; in the general case, I expect one would want identifiers to be included in the output JSON. We should definitely have something here that validates |
||
}, | ||
}, | ||
}, | ||
|
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.
We shouldn't add this without a proper design review, especially since this is affecting a public API.
I am also personally not a fan of this pattern - I think we should be really strict about what goes in this property bag and should limit to things to very core concepts.
I would prefer to extend the
toJsonSchema
transformation to allow the user to provide a callback that can be used to adjust the output based on their own custom metadata. This was the intention whencustom
was added here.The idea is that this would closely resemble
JSON.stringify
's replacer function parameter.https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify
I've had this work on my plate for a little while now, but have been consumed by website work 🫤
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.
Can our demo work okay without these changes? I'd prefer to have a separate PR add the relevant capabilities to the
toJsonSchema
transformation logic with the appropriate test coverage and everything. Would probably be easiest (if possible) to omit these changes for now and move forward with everything else.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.
It can work. I've removed the omitFromJson changes in commit 7bfba91ce95a5532c21588b8e7e4b3b750edeecc
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.
Yes our demo can work without this. I've removed the omitFromJson addition in commit 7bfba91
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.
Thanks! Let me know if/when you need the filtering functionality / want to discuss design options. I'm hoping that I'll be able to get back to SharedTree work next sprint, but I also thought I would get to it this sprint, so... 🤷♂️