-
Notifications
You must be signed in to change notification settings - Fork 533
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
feat: Publish ObjectNodeSchema #23090
base: main
Are you sure you want to change the base?
Conversation
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
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.
Code Coverage Summary
↑ packages.dds.tree.src.simple-tree.api:
Line Coverage Change: 0.01% Branch Coverage Change: No change
Metric Name | Baseline coverage | PR coverage | Coverage Diff |
---|---|---|---|
Branch Coverage | 88.76% | 88.76% | → No change |
Line Coverage | 82.35% | 82.36% | ↑ 0.01% |
↑ packages.dds.tree.src.simple-tree:
Line Coverage Change: 0.03% Branch Coverage Change: 0.01%
Metric Name | Baseline coverage | PR coverage | Coverage Diff |
---|---|---|---|
Branch Coverage | 94.07% | 94.08% | ↑ 0.01% |
Line Coverage | 97.23% | 97.26% | ↑ 0.03% |
Baseline commit: 9485e13
Baseline build: 306922
Happy Coding!!
Code coverage comparison check passed!!
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: 9485e13 |
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 20 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- packages/dds/tree/api-report/tree.legacy.public.api.md: Evaluated as low risk
- packages/dds/tree/src/simple-tree/api/tree.ts: Evaluated as low risk
- packages/dds/tree/src/simple-tree/objectNode.ts: Evaluated as low risk
- packages/dds/tree/src/index.ts: Evaluated as low risk
- packages/dds/tree/src/simple-tree/index.ts: Evaluated as low risk
- packages/dds/tree/src/simple-tree/api/schemaFactory.ts: Evaluated as low risk
- packages/dds/tree/src/simple-tree/objectNodeTypes.ts: Evaluated as low risk
- packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts: Evaluated as low risk
- packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md: Evaluated as low risk
- packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md: Evaluated as low risk
- packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.public.api.md: Evaluated as low risk
- packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md: Evaluated as low risk
- packages/framework/fluid-framework/api-report/fluid-framework.public.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.alpha.api.md: Evaluated as low risk
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
|
||
[`SchemaFactory.object`](https://fluidframework.com/docs/api/v2/tree/schemafactory-class#object-method) now returns an `ObjectNodeSchema` which exposes a `.fields` property contains a map from its property names to its [`FieldSchema`](https://fluidframework.com/docs/api/v2/tree/fieldschema-class). | ||
|
||
Additionally `schema instanceof ObjectNodeSchema` can be used to narrow a `TreeNodeSchema` to an `ObjectNodeSchema`. |
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'm still not sure how I feel about recommending usage of instanceof
, even when it is supported. Do we also have a type-narrowing function we can recommend?
I am all for supporting instanceof
to support consumers who choose to use it, but it doesn't seem like a best practice for us to recommend IMO.
@@ -726,6 +728,7 @@ export class SchemaFactory< | |||
* `error TS2589: Type instantiation is excessively deep and possibly infinite.` | |||
* which otherwise gets reported at sometimes incorrect source locations that vary based on incremental builds. | |||
*/ | |||
// TODO |
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.
Before merging?
/** | ||
* Extra data provided on all {@link ObjectNodeSchema} that is not included in the (soon possibly public) ObjectNodeSchema type. | ||
* Extra data provided on all {@link ObjectNodeSchema} in addition to the public ObjectNodeSchema type. |
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 (to avoid having to specify "ObjectNodeSchema" twice)
* Extra data provided on all {@link ObjectNodeSchema} in addition to the public ObjectNodeSchema type. | |
* Extra data provided on all {@link ObjectNodeSchema} beyond what is exposed by that type. |
or something
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.
Code changes look good to me. My only concern is recommending consumers use instanceof
. Curious what others think, but I generally think of instanceof
as an antipattern.
Description
This tweaks how ObjectNodeSchema is used so it can work with objectRecursive, then makes it public.
This provides much better access to field schema, which us very useful when implementing logic like schema dependency inversions (where the set of schema allowed in a field might not be known at compile time) or generic node processing (where which schema a node has isn't know at compile time).
This API has been very useful internally, and as we support more cases where the schema is unknown at compile time, this is starting to be useful externally as well.
Reviewer Guidance
The review process is outlined on this wiki page.