tree2: Refactor TreeNodeSchema #18217
Conversation
github-actions
bot
added
area: dds
…to CleanupSchema
…to CleanupSchema
…to CleanupSchema
|
This commit 66810bd is on my fork's branch, and fixes the conflicts. It is not showing up in the PR for some reason. Edit: pushing another commit seemed to nudge github into updating. |
…to CleanupSchema
⯅ @fluid-example/bundle-size-tests: +512 Bytes
Baseline commit: 4f6f622 |
|
CLA bot seems broken. Close and reopen to try and fix. |
experimental/dds/tree2/src/feature-libraries/typed-schema/typedTreeSchema.ts
Outdated
Show resolved
Hide resolved
experimental/dds/tree2/src/feature-libraries/node-key/nodeKeyIndex.ts
Outdated
Show resolved
Hide resolved
| export abstract class TreeNodeSchemaBase< | ||
| const out Name extends string = string, | ||
| const out Specification = unknown, | ||
| > implements TreeNodeStoredSchema |
There was a problem hiding this comment.
What is the implicication and reason for the change to have TreeNodeSchema and TreeFieldSchema implement *StoredSchema? It is not mentioned in the PR description.
There was a problem hiding this comment.
It did so before, and removing it breaks some code. For example it would require a an API to convert to stored schema, and a few places to use it. Contextually typed data is one example, dynamic schema validation (once we have it) will be another place that can work with both, and would need a conversion if view schema don't implement stored.
| @@ -165,9 +165,11 @@ describe("ContextuallyTyped", () => { | |||
| libraries: [leaf.library], | |||
| }); | |||
|
|
|||
| const nodeSchema = builder.objectRecursive("node", { | |||
| const recursiveReference = () => nodeSchema; | |||
| builder.fixRecursiveReference(recursiveReference); | |||
There was a problem hiding this comment.
Oops, I forgot to revert this change since this pattern tends to crash the stop working if you use the resulting schema very much. I think it ends up being order dependent across files in some broken way or something.
There was a problem hiding this comment.
I guess this one works so I can leave it.
Description
Cleanup TreeNodeSchema for Schema2 changes.
Breaking Changes
TreeNodeSchema is now a union of classes, with ObjectNodeSchema and FieldNodeSchema properly separated.
Reviewer Guidance
The review process is outlined on this wiki page.