Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new TreeAlpha.context(node) API that provides a unified interface for working with both hydrated (in-document) and unhydrated (newly created) tree nodes. The key innovation is the introduction of a three-tier hierarchy: TreeContextAlpha (most general) → TreeBranchAlpha → TreeViewAlpha (most specific), where each level adds capabilities that require more context.
Changes:
- Adds
TreeContextAlphainterface withrunTransaction,runTransactionAsync, andisBranch()methods that work for both hydrated and unhydrated nodes - Deprecates
TreeAlpha.branch()in favor of the newTreeAlpha.context()+context.isBranch()pattern - Refactors transaction types to introduce a
WithValue<T>interface that simplifies the transaction API for common cases
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dds/tree/src/simple-tree/api/tree.ts | Defines TreeContextAlpha interface and updates TreeBranchAlpha to extend it; adds deprecation notice to TreeAlpha.branch |
| packages/dds/tree/src/shared-tree/treeAlpha.ts | Implements TreeAlpha.context() and UnhydratedTreeContext class for unhydrated nodes |
| packages/dds/tree/src/shared-tree/schematizingTreeView.ts | Implements isBranch() method on SchematizingSimpleTreeView |
| packages/dds/tree/src/simple-tree/api/transactionTypes.ts | Introduces WithValue<T> interface and refactors transaction result types to use it |
| packages/dds/tree/src/test/simple-tree/treeBranch.spec.ts | Adds comprehensive test suite for branch functionality (new file) |
| packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts | Adds tests for the new context() API |
| packages/dds/tree/src/simple-tree/api/index.ts | Exports new TreeContextAlpha and WithValue types |
| packages/dds/tree/src/simple-tree/index.ts | Exports new types from api layer |
| packages/dds/tree/src/index.ts | Exports new types to public API surface |
| packages/dds/tree/api-report/tree.alpha.api.md | API report reflecting new types and deprecations |
| packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md | API report for fluid-framework package |
| .changeset/green-sides-watch.md | Changeset documentation with migration guide |
| private static wrapTransactionResult<TValue>( | ||
| value: WithValue<TValue> | void, | ||
| ): TransactionResultExt<TValue, TValue> | TransactionResult { | ||
| if (value?.value !== undefined) { |
There was a problem hiding this comment.
The condition value?.value !== undefined will incorrectly treat {value: 0}, {value: false}, {value: ""}, and {value: null} as if no value was returned. This should check value !== undefined && "value" in value instead to properly distinguish between a void return and a WithValue return.
There was a problem hiding this comment.
Huh? Am I crazy, or are you?
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
This adds a new tree static function
Tree.context(node)which yields a new type: aTreeContext. ATreeContextis a further generalization of branch and view. So the hierarchy (from most general to most specific) is now:TreeContext- A place for functions that don't require the caller to have hydrated/attached content - i.e., the ST DDS / Fluid are not necessarily reachable.TreeBranch- A place for functions that do require the caller to have hydrated content, and can reach the ST/Fluid.TreeView- A schematized tree branchAccordingly, you use
isBranch()to downcast aTreeContextto aTreeBranch, and thenhasRootSchemato downcast aTreeBranchto aTreeView.TreeContextis now the home ofrunTransaction, sincerunTransactioncan operate on both hydrated and unhydrated nodes - in the latter case, it simply runs the transaction delegate. This lets you doTree.context(node).runTransaction(...)without having to worry about whethernodeis hydrated or not. Note that this "upcasted" version ofrunTransactiondoes not have any options (like labels, or constraints) because those don't make sense until you downcast to a branch.Tree.branchis now deprecated in favor ofTree.context()+context.isBranch(). This is slightly more verbose for some situations but it is trivial for a consumer to write a helper to replicate the previous behavior ofTree.branchif they want the one-liner. By removingTree.branch, we avoid an awkward naming dilemma: most otherTree.*functions are single nouns which retrieve that noun, and the same is true forbranch- but "branch" is also a verb, which makes it sound like the API is forking.