feat(tree): Add the ability to associate metadata with Node Schema

[Josmithr]

Introduces an options parameter like we support for Field Schema, but for Node Schema. Metadata is propagated to the resulting Node Schema and is available via a readonly metadata property.

Example:

class Point extends withMetadata(
	schemaFactory.object("Point", {
		x: schemaFactory.required(schemaFactory.number),
		y: schemaFactory.required(schemaFactory.number),
	}),
	{
		description: "A point in 2D space",
	},
) {}

Note: the above is only available for class-based schema, and not structural node schema. There are technical reasons why the latter is more difficult to implement, and it isn't clear that the latter scenario is particularly valuable given that we generally recommend the former pattern when possible. We can always revisit this later as needed based on user need.

Also updates the simple schema and json schema domains and associated transformations to respect the built-in description property.

Follow-up to #22538

[seanimam]

Worth explaining what 'the document' here is. I imagine this might confuse end users. I'm also wondering if we should explain why you'd want to include metadata in the first place in this comment as well.

[Josmithr]

I've updated this to include a bit more detail, but have intentionally removed the term "document", as I don't think it's actually relevant to this. The important thing to note is that the data does not get persisted and is application-local only. As a result, changes to the data will not affect other collaborators.

[Josmithr]

@noencke interested in your thoughts on the specific terminology used here. Would probably be useful to ensure it aligns with how we document these concepts elsewhere.

[noencke]

Hmm. My first instinct is to call it "the document". That's definitely been the terminology used in SharedTree over the years. But I'm not sure if it agrees with broader FF. Saying "the file" is overly prescriptive (and possibly wrong). Saying "the summary" sounds a little too technical to me. "Container" isn't right either. "Document" lines right up with what we want IMO - it means that some data has been "documented" (i.e. written down) - nothing more and nothing less.

[alexvy86]

I think this is even clearer? "Local to the application" could be a bit confusing since Fluid applications are built on top of client-server interactions. Would even consider something like "local to a given instance of a Fluid container / instance of SharedTree" or something along those lines.

Suggested change

	* Note: this metadata is not persisted; it is strictly local to the application.
	* Note: this metadata is not persisted; it is strictly local to the client.

[Josmithr]

Except that this data never even makes it to Fluid. It really is local to the instance of the node schema. I'm not sure what would make this the most clear, unless we already have documentation for similar "ephemeral" concepts.

[alexvy86]

It gets verbose, but how about something like this: "Note: this metadata is not persisted nor made part of the collaborative state. Multiple instances of the application (e.g. multiple users across several computers) connected to the same Fluid container could all be using the same metadata if they're all running the same version of the code, but they could as easily be using different metadata if they're running different versions of the code (e.g. during a rollout of a new version)."

[Josmithr]

Yeah, something like that sounds good. I'll tweak the wording a bit, but something to the same effect.

[Josmithr]

Updated. I went with something more verbose than what I had, but slightly less verbose than your suggestion. Let me know what you think.

[CraigMacomber]

The way this API was added requires modifying the signatures of all the schema production APIs.
This doesn't scale great (one feature, many APi changes, lots of parameters that need docs).

An API like

class Foo extends sf.object(....).withMetadata({...}) {}

Should be possible, and seems more orthogonal.

Specifically a withMetadata method on TreeNodeSchemaClass which returns a subclass of the "this" parameter with the metadata added.

[Josmithr]

Interesting. I like how this scales. I can play around with this.

Questions:

1. Are any caching behaviors / identifier invariants broken by this? E.g., what happens if a developer does something like...

   class Foo extends sf.object("Foo", {...}) { }
   class Foo2 extends Foo.withMetadata({...}) { }

   Both schemas have the same identifier ("Foo"), so mixing and matching them seems unsafe. Is that right?

2. Would it be safe to introduce the same pattern for field schema? Same questions as #1 for this.

[noencke]

I would like to explore the "fluent-style" (i.e. chaining methods that return this) syntax for our schema APIs. In the past we've recognized that it could also be useful for parameterizing fields (e.g. sf.optional(...).withDefault(...).withEtc(...)). It might be a convenient way to decompose the giant bag of properties that currently lives in the options parameter. However, we'd need to think it through and review it, since it'd be a significant departure from the current API. In the meantime, this PR's syntax matches the way we do options for field schema, so it is consistent. If we want to add a fluent API, we can update both at once (node options and field options) in the future.

[CraigMacomber]

For classes, we allow subclassing as long as there is only a single leaf used as the schema. Since we can't tell what that leaf is until its actually used, nothing should be broken by this pattern caching wise. See TreeNodeValid.constructorCached for details.

[CraigMacomber]

For field schema, we don't care about object identity, so no issue there. However we can't put a method on ImplicitFieldSchema (which can be an array), so I think having it work via the FieldSchema constructor makes sense, though we could also add a method to FieldSchema's to replace the metadata (copy the schema with new metadata), Given our comparison semantics for field schema, that should work fine.

[noencke]

TS-assisted "fluency/builder type", as a fun POC.

[CraigMacomber]

If you want to have time to explore options before stabilizing something, you can take the fluent/method approach, but for now make it a free function so it can be @alpha and not forced to be public (it can be implemented by calling a non publicly exposed method, which could be made public to stabilize the API).

Generally I'd like to avoid arguments after the fields list since any parameters after one which is wrapped onto multiple lines makes for hard to read code. As this new APi violates that, I consider it a significant departure from our existing patterns, and impacting a bunch of methods when doing so. I'd rather have a small single new API which might be a bit odd: if we decide the method is bad and want to do it some other way, its easy to keep supported and doesn't clutter up the factory API methods with even harder to read type signatures and less clear overloading (when we have parameter count based overloads, adding optional parameters can be problematic).

Thus I don't really like adding a bunch of optional arguments to these methods, and if we do it this way, then decide we don't like it, we are left with a worse situation since deprecating several method parameters is harder/messier (and more customer impacting) than one method. In short the cost of making the wrong call on how we want the API is a lot lower if we go with the method on TreeNodeSchemaClass. Thus even if we might ultimately want the extra parameter to the factory methods, I'd err on the side of the much smaller and easier to document and deprecate APi surface of the TreeNodeSchemaClass method. Also if we decide we want a different version of this API, we can always add more methods: adding more optional parameters doesn't really work.

[tylerbutler]

Suggested change

	Functionality like the experimental conversion of Tree Schema to [JSON Schema](https://json-schema.org/) (`getJsonSchema`) leverages such system-understood metadata to generate useful information.
	Functionality like the experimental conversion of Tree Schema to [JSON Schema](https://json-schema.org/) (`getJsonSchema`) uses such system-understood metadata to generate useful information.

[Josmithr]

I guess I'm not sure why the latter is better here. Both are "correct", where my wording is technically a bit more specific. I don't really have a strong opinion either way, but I don't see the benefit in the change.

[Josmithr]

Unrelated to my changes, but fixes a linter warning.

[github-actions]

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output

> [email protected] 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"


> [email protected] serve
> docusaurus serve --no-open

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

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170006 links
    1595 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors

[github-actions]

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output

> [email protected] 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"


> [email protected] serve
> docusaurus serve --no-open

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

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170006 links
    1595 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors

[Josmithr]

Replaced with #23321