Conversation
github-actions
bot
added
area: dds
There was a problem hiding this comment.
Pull Request Overview
This PR fixes incorrect type parameters in SchemaFactoryBeta's object and objectRecursive methods, specifically addressing insufficiently specific types for schema metadata. The return types now properly include never and TCustomMetadata parameters that were previously missing.
Key changes:
- Updated
TreeNodeSchemaClassreturn type parameters inSchemaFactoryBetamethods to include metadata types - Fixed test assertions that were incorrectly accessing metadata properties directly without null checks
- Split test cases to separately validate both Beta and Alpha factory behaviors
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/framework/fluid-framework/api-report/fluid-framework.legacy.beta.api.md | Updated API signatures for object and objectRecursive methods with corrected type parameters |
| packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md | Updated API signatures for object and objectRecursive methods with corrected type parameters |
| packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md | Updated API signatures for object and objectRecursive methods with corrected type parameters |
| packages/dds/tree/src/test/simple-tree/api/schemaFactoryRecursive.spec.ts | Split metadata test into separate beta and alpha cases with proper null checks |
| packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts | Added test coverage for customized classes and split metadata tests for beta and alpha factories |
| packages/dds/tree/src/simple-tree/api/schemaFactoryBeta.ts | Added missing never and TCustomMetadata type parameters to return types |
| packages/dds/tree/api-report/tree.legacy.beta.api.md | Updated API signatures for object and objectRecursive methods with corrected type parameters |
| packages/dds/tree/api-report/tree.beta.api.md | Updated API signatures for object and objectRecursive methods with corrected type parameters |
| packages/dds/tree/api-report/tree.alpha.api.md | Updated API signatures for object and objectRecursive methods with corrected type parameters |
| 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. |
There was a problem hiding this comment.
this was the issue!
| type Instance = InstanceType<typeof NoteCustomized>; | ||
| type _checkInstance = requireTrue<areSafelyAssignable<Instance, NoteCustomized>>; | ||
|
|
||
| type Test = TreeNodeFromImplicitAllowedTypes<typeof NoteCustomized>; |
There was a problem hiding this comment.
Nit: any reason this type isn't in-lined like the subsequent test cases?
There was a problem hiding this comment.
I probable needed to mouse over it to investigate things at some point
packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts
Outdated
Show resolved
Hide resolved
packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts
Outdated
Show resolved
Hide resolved
| assert.deepEqual(Foo.metadata, fooMetadata); | ||
| }); | ||
|
|
||
| it("Node schema metadata - alpha", () => { |
There was a problem hiding this comment.
Not necessarily for this PR, but should we be running most of these tests over all of the applicable variants of SchemaFactory? Probably largely redundant, but might be nice to catch unexpected typing differences between the implementations.
There was a problem hiding this comment.
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.
packages/dds/tree/src/test/simple-tree/api/schemaFactoryRecursive.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| 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. |
There was a problem hiding this comment.
| // 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. |
packages/dds/tree/src/test/simple-tree/api/schemaFactoryRecursive.spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output |
## Description SchemaFactoryBeta incorrectly gave insufficiently specific types for schema metadata, and passed tests due to a bug in how the tests were written.
Description
SchemaFactoryBeta incorrectly gave insufficiently specific types for schema metadata, and passed tests due to a bug in how the tests were written.
Reviewer Guidance
The review process is outlined on this wiki page.