tree: Improve non-schema aware typing #22763

CraigMacomber · 2024-10-08T23:46:40Z

Description

See changeset

Breaking Changes

See changeset

Reviewer Guidance

The review process is outlined on this wiki page.

This is a rather risky change, as it could break some use-cases due to typing changes. These are use cases we never really intended to support, and there should be alternatives, but this could cause some breakage when integrating.

Extra care should be given to evaluating if this is ok to release in a minor version. A prerelease build, and testing by some representative customers would likely be a good idea.

packages/dds/tree/src/simple-tree/arrayNode.ts

packages/dds/tree/src/simple-tree/schemaTypes.ts

.vscode/settings.json

CraigMacomber · 2024-10-09T00:54:27Z

packages/dds/tree/src/simple-tree/core/treeNodeSchema.ts

@@ -19,7 +20,7 @@ import type { InternalTreeNode, Unhydrated } from "./types.js";
 export type TreeNodeSchema<
 	Name extends string = string,
 	Kind extends NodeKind = NodeKind,
-	TNode = unknown,
+	TNode extends TreeNode | TreeLeafValue = TreeNode | TreeLeafValue,


This is really the main part of this change. Pretty much everything else is done to allow this change to take place.

msfluid-bot · 2024-10-09T16:23:25Z

⯅ @fluid-example/bundle-size-tests: +305 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	459.85 KB	459.89 KB	⯅ +35 Bytes
azureClient.js	557 KB	557.04 KB	⯅ +49 Bytes
connectionState.js	724 Bytes	724 Bytes	■ No change
containerRuntime.js	259.47 KB	259.48 KB	⯅ +14 Bytes
fluidFramework.js	405.81 KB	405.85 KB	⯅ +44 Bytes
loader.js	134.16 KB	134.18 KB	⯅ +14 Bytes
map.js	42.46 KB	42.46 KB	⯅ +7 Bytes
matrix.js	148.29 KB	148.29 KB	⯅ +7 Bytes
odspClient.js	523.96 KB	524 KB	⯅ +49 Bytes
odspDriver.js	97.84 KB	97.86 KB	⯅ +21 Bytes
odspPrefetchSnapshot.js	42.81 KB	42.82 KB	⯅ +14 Bytes
sharedString.js	164.48 KB	164.49 KB	⯅ +7 Bytes
sharedTree.js	396.27 KB	396.31 KB	⯅ +37 Bytes
Total Size	3.31 MB	3.31 MB	⯅ +305 Bytes

Baseline commit: 5c17a9c

Generated by 🚫 dangerJS against 0160982

.changeset/shiny-swans-relax.md

packages/dds/tree/src/simple-tree/api/tree.ts

packages/dds/tree/src/simple-tree/core/treeNodeSchema.ts

packages/dds/tree/src/simple-tree/arrayNode.ts

packages/dds/tree/src/simple-tree/schemaTypes.ts

packages/dds/tree/src/simple-tree/toMapTree.ts

Co-authored-by: Noah Encke <[email protected]>

…Framework into BetterTypes2

packages/dds/tree/src/simple-tree/api/typesUnsafe.ts

Josmithr · 2024-10-21T20:39:44Z

packages/dds/tree/src/simple-tree/api/typesUnsafe.ts

@@ -83,6 +142,14 @@ export type TreeFieldFromImplicitFieldUnsafe<TSchema extends Unenforced<Implicit
 			? TreeNodeFromImplicitAllowedTypesUnsafe<TSchema>
 			: unknown;

+/**
+ * {@link Unenforced} version of {@link AllowedTypes}.
+ * @remarks


Not necessarily for this PR, but should we add similar notices to the existing Unsafe types here as well?

Its redundant with the system tag: I'd prefer a more general/automated solution to convey that to intelisense users.

Josmithr · 2024-10-21T20:43:38Z

packages/dds/tree/src/simple-tree/schemaTypes.ts

@@ -381,15 +382,25 @@ export type ImplicitAllowedTypes = AllowedTypes | TreeNodeSchema;
 export type ImplicitFieldSchema = FieldSchema | ImplicitAllowedTypes;

 /**
- * Converts ImplicitFieldSchema to the corresponding tree node's field type.
+ * Converts an `ImplicitFieldSchema` to a property type suitable for reading a field with this that schema.


Should this type be @system?

Making this system would be a breaking change (it removes app's ability to use this), so we can't do that, regardless of if we think apps need it.

packages/dds/tree/src/simple-tree/schemaTypes.ts

packages/dds/tree/src/test/simple-tree/object.spec.ts

Josmithr · 2024-10-21T20:48:02Z

packages/dds/tree/src/test/simple-tree/object.spec.ts

+/**
+ * Map a generic function over an array, assuming that this is a well defined operation.
+ * @remarks
+ * This is useful for when a collection of generic interface value with differing values for their type parameter got collected in a single type erased array, but you want to process them generically.


This is a bit hard to parse, IMO. Might be worth restructuring a bit.

Josmithr · 2024-10-21T20:49:26Z

packages/framework/ai-collab/src/shared-tree-diff/sharedTreeBranchManager.ts

@@ -183,7 +184,10 @@ export class SharedTreeBranchManager {
 			switch (diff.type) {
 				case "CHANGE":
 				case "CREATE": {
-					targetObject.set(diff.path[diff.path.length - 1] as string, diff.value);
+					// This code is doing non-schema aware editing, which is not a supported feature of this API.


Should we leave a TODO to not do this? Or is this intentional and desired? Not clear to me from the docs.

I don't have a lot of context on this ai-collab code. My understanding was that we don't support non-schema aware editing, but this code is doing exactly that. We should probably do one of:

support such a use case

stop depending on such usage

decide this code is a kludge

This package was added after I created this PR, and does not compile with my changes, so I looked at why and found it doing things we do not support which are now correctly blocked from compiling.

I this package is believe based on FHL work. Someone who owns this is going to need to provide some guidance here.

Decent chance it's #3. Not sure if the way we're headed (what we're calling the "explicit approach") we'll still need this code. @seanimam might be able to comment. If we keep the code, I suspect we'll want to figure out how to make this schema-aware.

Co-authored-by: Joshua Smithrud <[email protected]>

alexvy86

ai-collab changes are fine

.changeset/shiny-swans-relax.md

alexvy86 · 2024-10-21T22:13:58Z

packages/framework/ai-collab/api-report/ai-collab.alpha.api.md

@@ -105,6 +105,6 @@ export class SharedTreeBranchManager {
 export function sharedTreeDiff(obj: Record<string, unknown> | unknown[], newObj: Record<string, unknown> | unknown[], options?: Options, _stack?: (Record<string, unknown> | unknown[])[]): Difference[];

 // @alpha
-export function sharedTreeTraverse<T = unknown>(jsonObject: TreeMapNode | TreeArrayNode | Record<string, unknown> | unknown[], path: ObjectPath): T | undefined;
+export function sharedTreeTraverse<T = unknown>(jsonObject: TreeMapNode | TreeArrayNode | Record<string, unknown>, path: ObjectPath): T | undefined;


No issues here. Besides being an @alpha API, we're in the process of redesigning this library's whole API anyway.

packages/framework/ai-collab/src/shared-tree-diff/sharedTreeBranchManager.ts

alexvy86 · 2024-10-21T22:16:44Z

packages/framework/ai-collab/src/shared-tree-diff/sharedTreeBranchManager.ts

@@ -183,7 +184,10 @@ export class SharedTreeBranchManager {
 			switch (diff.type) {
 				case "CHANGE":
 				case "CREATE": {
-					targetObject.set(diff.path[diff.path.length - 1] as string, diff.value);
+					// This code is doing non-schema aware editing, which is not a supported feature of this API.


Decent chance it's #3. Not sure if the way we're headed (what we're calling the "explicit approach") we'll still need this code. @seanimam might be able to comment. If we keep the code, I suspect we'll want to figure out how to make this schema-aware.

Co-authored-by: Alex Villarreal <[email protected]>

Co-authored-by: Joshua Smithrud <[email protected]>

CraigMacomber · 2024-10-22T17:04:17Z

I'm skipping the code coverage check since only simple-tree/api (the folder) regressed, and no files regressed. I'm going to assume this is because typesUnsafe.ts is longer, and the tool seems to consider it uncovered since its entirely type definitions.

github-actions · 2024-10-22T18:41:07Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> [email protected] ci:start
> http-server ./public --port 1313 --silent


> [email protected] linkcheck:full
> npm run linkcheck:fast -- --external


> [email protected] linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  428384 links
    3310 destination URLs
       2 URLs ignored
       0 warnings
       0 errors

…22938) ## Description This improves on #22763 to be slightly more specific in the case of TreeNodeSchemaClass with an unspecified TNode parameter.

Improve non-schema aware typing

c6bff5f

CraigMacomber requested review from a team as code owners October 8, 2024 23:46

CraigMacomber commented Oct 9, 2024

View reviewed changes

packages/dds/tree/src/simple-tree/arrayNode.ts Outdated Show resolved Hide resolved

CraigMacomber commented Oct 9, 2024

View reviewed changes

packages/dds/tree/src/simple-tree/schemaTypes.ts Show resolved Hide resolved

Apply suggestions from code review

be09116

CraigMacomber commented Oct 9, 2024

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

CraigMacomber commented Oct 9, 2024

View reviewed changes

CraigMacomber added 2 commits October 9, 2024 08:21

Fix API-extractor

0b3e0f2

Update .vscode/settings.json

2094876

github-actions bot added area: contributor experience and removed area: contributor experience labels Oct 9, 2024

Merge branch 'main' into BetterTypes2

54b9730

github-actions bot removed the area: contributor experience label Oct 9, 2024

noencke reviewed Oct 9, 2024

View reviewed changes

CraigMacomber and others added 6 commits October 9, 2024 11:55

Apply suggestions from code review

24c9042

Co-authored-by: Noah Encke <[email protected]>

more privateRemarks

56b1da9

work on docs for TreeFieldFromImplicitField

11b9881

Update packages/dds/tree/src/simple-tree/arrayNode.ts

4e0913d

Co-authored-by: Noah Encke <[email protected]>

Add ReadonlyArrayNode limitation note

c768c55

Merge branch 'BetterTypes2' of https://github.com/CraigMacomber/Fluid…

8d4c75b

…Framework into BetterTypes2