refactor: Store data in unhydrated nodes directly instead of wrapping MapTree nodes #24739

CraigMacomber · 2025-05-30T20:05:22Z

Description

Implement map tree APIs with UnhydratedFlexTreeNode directly instead of a wrapping a MapTree.

Store contextual default providers directly in a strongly typed way in the tree. Implicitly generate them using a global context if requested while unhydrated instead of special casing identifiers in the read code paths in multiple places.

Provide contextual defaults as part of prepare for hydration pass instead of its own pass.

Overall, this refactor should make the handling un unhydrated data simpler and more robust:

It is no longer possible for the unhydrated tree and map tree and/or the maps associating them to be out of sync.
The special case handling of identifiers has been implemented in a general way that will handle any contextual defaults, and no longer needs special logic in any code path reading from the tree.
There is no longer questionable ownership of ExclusiveMapTree nodes due to cache lookup.
Memory use should be slightly reduced by avoiding having the separate map tree objects and the caches to associate them with the unhydrated nodes.

Breaking Changes

Defaulted Identifiers show up when enumerating fields on an unhydrated node.

This seems like an improvement/simplification (makes unhydrated and hydrated nodes more similar, and identifiers less special), but could theoretically break something.

Reviewer Guidance

The review process is outlined on this wiki page.

…to unhydratedRefactor

## Description Cleanups split out of #24739

…to unhydratedRefactor

CraigMacomber · 2025-06-02T16:40:29Z

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

@@ -10,30 +10,24 @@ import { isFluidHandle } from "@fluidframework/runtime-utils/internal";
 import {


This file and its functions have not been renamed yet to help simplify the diff.
A follow-up change will rename them.

CraigMacomber · 2025-06-02T17:31:54Z

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

@@ -1112,8 +1114,6 @@ export class SchemaFactory<
 	 *
 	 * - A compressed form of the identifier can be accessed at runtime via the {@link TreeNodeApi.shortId|Tree.shortId()} API.
 	 *
-	 * - It will not be present in the object's iterable properties until explicitly read or until having been inserted into a tree.


This removal is the potentially breaking change noted in the changeset.

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

Josmithr · 2025-06-02T20:11:25Z

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

+	/**
+	 * Cache storing the TreeNode for this inner node.
+	 */
+	public treeNode: TreeNode | undefined = undefined;


Can the write access to this be restricted? If not, what are the rules / expectations for external setting of this?

Is there ever a time where we expect the value to be set back to undefined (externally) after being populated? If not, would a setter that doesn't accept undefined be appropriate?

I have clarified the comment slightly, but otherwise left this as is.

How flex trees get associated with TreeNodes is really an implementation detail of the code which makes TreeNodes, (currently TreeNodeKernel), and this code shouldn't care. It's providing this field for use by TreeNodeKernel, which, as an implementation detail, currently will never set this back to undefined.

I don't think making this a private property then adding getters and setters with strictly enforces rules really would simplify things over the existing code: it would just move the policy from tree node kernel to living in both places.

These classes are mostly used as implementation of FlexTreeNode, and properties like this one which are only exposed in the implementation and not via the interface are only used by code which is implementing unhydrated node specific logic, which there shouldn't be much of (especially after this change). This isn't a type that's at all wildly used and the total set of places with access to this property is very small.

Users who are not part of the implementation logic of unhydrated nodes will just use getOrCreateNodeFromInnerNode. We can't encapsulate that get or create logic inside of the getter here as that would be a cyclic dep and break our layering. Adding a whole getter+setter wrapper to place a limit of the setter while still getting the getter return undefined when not generated yet just doesn't seem like a real safety win worth the runtime access overhead and extra logical coupling.

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

Josmithr · 2025-06-02T20:12:48Z

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


 	public constructor(
-		public readonly simpleContext: Context,
+		public readonly context: FlexTreeContext,


This is part of an interface implementation and thus implicitly gets inherited documentation.

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

Josmithr · 2025-06-02T20:14:11Z

packages/dds/tree/src/simple-tree/node-kinds/object/objectNode.ts

-				unhydratedContext,
-				mapTreeFromNodeData(input as object, this as unknown as ImplicitAllowedTypes),
-			);
+			return mapTreeFromNodeData(input as object, this as unknown as ImplicitAllowedTypes);


Nit: a quick comment to explain the cast through unknown would probably be good

Tweaked to avoid casting through unknown

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

.changeset/fruity-laws-rule.md

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

noencke · 2025-06-02T22:48:51Z

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

-				0xa27 /* Expected hydrated identifier */,
-			);
-			const identifierValue = identifier.value as string;
+			const identifierField = flexNode.tryGetField(key);


@daesunp FYI the logic has changed here - you may want to review

noencke · 2025-06-03T18:12:19Z

packages/dds/tree/src/util/utils.ts

+/**
+ * Returns true if and only if the given iterable has at least one element.
+ */
+export function iterableHasSome<T>(iterable: Iterable<T>): boolean {


It'd be slick if this were just another overload of hasSome (put it below the array: readonly T[] variant so it has lowest precedence). This implementation should work for all of the hasSome overloads. I know you are wary of overloads 😅, but in this case it seems like it'd be a net good.

noencke · 2025-06-03T18:16:59Z

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

 ): void {
 	if (hydratedData !== undefined) {
+		// Prepare content before validating side this populated defaults using the provided context rather than the global context.


I'm having trouble parsing this paragraph (but it's definitely better than no paragraph - thanks). It's trying to communicate two things?

That we apply the defaults (via prepareContentForHydration) before we validate against the schema, and

That we have more context available to us here than we do inside of schema validation, so it's important to apply the defaults with that context while we can

?

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

noencke · 2025-06-03T18:28:57Z

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

-	onEdit?: () => void,
+/** Creates a field with the given attributes */
+export function createField(
+	...args: [


How about

...args: ConstructorParameters<typeof UnhydratedFlexTreeField>

To implicitly explain why the arguments are like this

Thats way better. Done.

CraigMacomber added 10 commits May 23, 2025 15:22

Refactor UnhydratedFlexTree and toMapTree

ed52a5c

more fixes

3d031e2

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

adec2a8

…to unhydratedRefactor

Fix build

8a6c2a0

Fix test enumeration

0e9c6d1

Fix a bunch of tests

0d74576

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

5b2ca88

…to unhydratedRefactor

Fix more tests

e812c58

Fix more tests, mostly verbose tree validation

1c6350c

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

29e0fad

…to unhydratedRefactor

github-actions bot added base: main PRs targeted against main branch area: dds Issues related to distributed data structures area: dds: tree labels May 30, 2025

CraigMacomber mentioned this pull request May 30, 2025

Cleanup unhydrated tree and tree import code more generally #24740

Merged

CraigMacomber added a commit that referenced this pull request May 31, 2025

Cleanup unhydrated tree and tree import code more generally (#24740)

0762660

## Description Cleanups split out of #24739

CraigMacomber added 8 commits May 30, 2025 18:46

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

3367466

…to unhydratedRefactor

Fix from merge

78bdc8f

Fix test

6c94fef

Fix duplicate events

97aa5d7

Fix a few tests

234ff66

Fix remaining test errors

787f5c0

Merge branch 'main' into unhydratedRefactor

96b0472

Add changeset

b756abe

github-actions bot added the changeset-present label Jun 2, 2025

CraigMacomber changed the title ~~Unhydrated refactor~~ refactor: Store data in unhydrated nodes directly instead of wrapping MapTree nodes Jun 2, 2025

CraigMacomber commented Jun 2, 2025

View reviewed changes

CraigMacomber added 3 commits June 2, 2025 09:53

cleanup imports

f11ccfe

Replace replace unhydratedFlexTreeNodeToTreeNode with treeNode property

0b364b0

Merge branch 'main' into unhydratedRefactor

ca39e57

CraigMacomber marked this pull request as ready for review June 2, 2025 17:08

CraigMacomber requested review from a team as code owners June 2, 2025 17:08

CraigMacomber commented Jun 2, 2025

View reviewed changes

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

CraigMacomber added 2 commits June 2, 2025 11:13

simplify iteration

ea9a42e

Avoid copy in UnhydratedFlexTreeNode

02f9245

Josmithr reviewed Jun 2, 2025

View reviewed changes

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

Josmithr reviewed Jun 2, 2025

View reviewed changes

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

Josmithr reviewed Jun 2, 2025

View reviewed changes

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

Josmithr reviewed Jun 2, 2025

View reviewed changes

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

Josmithr reviewed Jun 2, 2025

View reviewed changes

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

Josmithr reviewed Jun 2, 2025

View reviewed changes

.changeset/fruity-laws-rule.md Show resolved Hide resolved

CraigMacomber added 4 commits June 2, 2025 13:43

Simplify cast

07a47f8

add some docs

db1306c

More and updated docs

8d7531f

SImplify and better document a few bits of unhydrated node logic

e320162

noencke reviewed Jun 2, 2025

View reviewed changes

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

noencke requested a review from daesunp June 2, 2025 22:48

noencke reviewed Jun 2, 2025

View reviewed changes

Document globalIdentifierAllocator

f45634c

noencke reviewed Jun 3, 2025

View reviewed changes

CraigMacomber added 2 commits June 3, 2025 15:39

Document lazyChildren

dabd054

Use ConstructorParameters

f0e50f3

		@@ -10,30 +10,24 @@ import { isFluidHandle } from "@fluidframework/runtime-utils/internal";
		import {

refactor: Store data in unhydrated nodes directly instead of wrapping MapTree nodes #24739

Are you sure you want to change the base?

refactor: Store data in unhydrated nodes directly instead of wrapping MapTree nodes #24739

Uh oh!

Conversation

CraigMacomber commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Breaking Changes

Reviewer Guidance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CraigMacomber commented May 30, 2025 •

edited

Loading