-
Notifications
You must be signed in to change notification settings - Fork 537
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
Expose SchemaFactoryAlpha as an alpha API #23362
Conversation
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.
Copilot reviewed 5 out of 14 changed files in this pull request and generated no comments.
Files not reviewed (9)
- packages/dds/tree/api-report/tree.legacy.public.api.md: Evaluated as low risk
- packages/dds/tree/src/index.ts: Evaluated as low risk
- packages/dds/tree/api-report/tree.beta.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.legacy.alpha.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.alpha.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.public.api.md: Evaluated as low risk
- .changeset/moody-pandas-fall.md: Evaluated as low risk
- packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts: Evaluated as low risk
- packages/dds/tree/src/simple-tree/index.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
packages/dds/tree/src/simple-tree/api/schemaFactoryAlpha.ts:90
- The method
object
is being called withinobjectRecursive
but it is not clear ifobject
supports the sameoptions
parameter. Ensure thatobject
method can handleSchemaFactoryObjectOptions
correctly.
return this.object(
@@ -103,6 +103,8 @@ export function schemaFromValue(value: TreeValue): TreeNodeSchema { | |||
|
|||
/** | |||
* Options when declaring an {@link SchemaFactory.object|object node}'s schema | |||
* | |||
* @alpha |
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: Question about the documentation on the property below. The first line reads:
Opt in to viewing documents which have unknown optional fields for this object's identifier, i.e. optional fields
in the document schema which are not present in this object schema declaration.
I'm not sure what "identifier" means in this context. Should this instead just say something like:
Allow nodes typed with this object node schema to contain optional fields that are not present in the schema declaration.
?
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 like that way of putting it. identifier
refers to the schema's identifier, but it's not particularly necessary to mention that here.
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.
Left a few suggestions but otherwise looks great to me! And lines up very well with unrelated schema factory functionality I'm working on.
You mentioned that API-Extractor was complaining about these changes earlier, but no longer seems to be? Let me know if I can help investigate anything there. But assuming it continues to be stable, looks good to me!
Yeah, I'm not totally sure it was actually an API-extractor issue, it just surfaced that way. I think the original problem I was seeing was typescript emitting 'any' which down the line caused problems, but it's plausible that when I last tried this I had missed something subtle (previous PR had a lot of changes) and when reimplementing this from a fresh state I didn't make the same mistake. |
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
Description
Exposes
SchemaFactoryAlpha
and associated additional options onobject
andobjectRecursive
to tree's alpha API surface. This lights up the feature implemented in #23192, so I have also added a changeset with some user-facing guidance on that feature.