Fix metadata types#25736

Merged

CraigMacomber merged 3 commits intomicrosoft:mainfrom

CraigMacomber:metaType

Oct 23, 2025

Contributor

CraigMacomber commented Oct 22, 2025

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.


          Fix metadata types

1b8e830

CraigMacomber requested a review from a team as a code owner

October 22, 2025 23:46

Copilot AI review requested due to automatic review settings

October 22, 2025 23:46

CraigMacomber requested a review from a team as a code owner

October 22, 2025 23:46

github-actions bot added area: dds area: dds: tree area: framework public api change labels

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a 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 TreeNodeSchemaClass return type parameters in SchemaFactoryBeta methods 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

github-actions bot added the base: main label

CraigMacomber commented

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts Show resolved Hide resolved


          Add order notes

361fb1f

CraigMacomber commented

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts Outdated

+              			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.

Contributor Author

CraigMacomber Oct 22, 2025

this was the issue!

Josmithr reviewed

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts

+              		type Instance = InstanceType<typeof NoteCustomized>;
+              		type _checkInstance = requireTrue<areSafelyAssignable<Instance, NoteCustomized>>;
+              		type Test = TreeNodeFromImplicitAllowedTypes<typeof NoteCustomized>;

Contributor

Josmithr Oct 23, 2025 •

edited

Loading

Nit: any reason this type isn't in-lined like the subsequent test cases?

Contributor Author

CraigMacomber Oct 23, 2025

I probable needed to mouse over it to investigate things at some point

Josmithr reviewed

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts Outdated Show resolved Hide resolved

Josmithr reviewed

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts Outdated Show resolved Hide resolved

Josmithr reviewed

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts

               			assert.deepEqual(Foo.metadata, fooMetadata);
+              		});
+              		it("Node schema metadata - alpha", () => {

Contributor

Josmithr Oct 23, 2025

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.

Contributor Author

CraigMacomber Oct 23, 2025

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.

Josmithr reviewed

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactoryRecursive.spec.ts Outdated Show resolved Hide resolved

Josmithr reviewed

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactoryRecursive.spec.ts Outdated


		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.

Contributor

Josmithr Oct 23, 2025

Suggested change

      
            			// 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.

Josmithr approved these changes

View reviewed changes

CraigMacomber commented

View reviewed changes

packages/dds/tree/src/test/simple-tree/api/schemaFactoryRecursive.spec.ts Outdated Show resolved Hide resolved


          Apply suggestions from code review

bfb32f3

Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>

Josmithr approved these changes

View reviewed changes

CraigMacomber enabled auto-merge (squash)

October 23, 2025 00:27

Contributor

github-actions bot commented Oct 23, 2025

🔗 Found some broken links! 💔

Run a link check locally to find them. See
https://github.com/microsoft/FluidFramework/wiki/Checking-for-broken-links-in-the-documentation for more information.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

 ELIFECYCLE  Command failed with exit code 1.

CraigMacomber merged commit 5797079 into microsoft:main

39 checks passed

CraigMacomber deleted the metaType branch

October 23, 2025 01:22

anthony-murphy-agent pushed a commit to anthony-murphy-agent/FluidFramework that referenced this pull request


          Fix metadata types (microsoft#25736)

4fb88cd

## Description

SchemaFactoryBeta incorrectly gave insufficiently specific types for
schema metadata, and passed tests due to a bug in how the tests were
written.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: dds: tree area: dds area: framework base: main public api change