Improve typing of TreeBeta.clone() #23553
Conversation
github-actions
bot
added
base: main
There was a problem hiding this comment.
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (3)
- packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md: Evaluated as low risk
- packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.beta.api.md: Evaluated as low risk
| Improved typing of `TreeBeta.clone` | ||
|
|
||
| The return type of `TreeBeta.clone` now matches the input type. | ||
| Existing usages which were supplying the generic parameter for the input/output type should remove the explicit generic parameter and allow it to be inferred from the input. |
There was a problem hiding this comment.
FWIW, we could make this a non-breaking change by keeping the old signature around as a second overload. However, since this is a beta API, it seems like it'd be best to just have any consumers be forced to fix it since it's so easy to fix and makes their code immediately nicer.
| assert.equal(clonedMetadata, topLeftPoint.metadata, "String not cloned properly"); | ||
| }); | ||
|
|
||
| // Compile-time test: ensure that the return type of clone is the same as the input type for common cases (nodes, primitives, undefined). | ||
| function preservesTypesWhenCloning( |
There was a problem hiding this comment.
Consider adding a test where a type is explicitly specified to validate that is working as expected.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
| @@ -808,7 +808,7 @@ export interface TreeArrayNodeUnsafe<TAllowedTypes extends Unenforced<ImplicitAl | |||
| // @beta @sealed | |||
| export const TreeBeta: { | |||
| on<K extends keyof TreeChangeEventsBeta<TNode>, TNode extends TreeNode>(node: TNode, eventName: K, listener: NoInfer<TreeChangeEventsBeta<TNode>[K]>): () => void; | |||
| clone<const TSchema extends ImplicitFieldSchema>(node: TreeFieldFromImplicitField<TSchema>): TreeFieldFromImplicitField<TSchema>; | |||
| clone<const TNode extends TreeFieldFromImplicitField<ImplicitFieldSchema>>(node: TNode): Unhydrated<TNode>; | |||
There was a problem hiding this comment.
This introduces a type unsoundness. We still might want to do it, but here is the reasoning why it was not done as you are suggesting in the first place:
As you have written it, it preserves all type information about the node being cloned. This could include information which is not part of the cloned data, and thus produce output which has in incorrect type,
For example consider a node which has a local optional function property "foo" that defaults to undefined.
If you do:
if (n.foo) {
const x = Tree.clone(n);
const result= x.foo(); // This compiles, but x.foo is undefined at runtime since the above type guard narrowed the type of n passed to clone.
}There was a problem hiding this comment.
Is that actually true? If n is e.g. a class-based schema node instance (which I think is what it would be in your example), then I don't think TS will capture n.foo's defined-ness in the type of n. It merely knows that n.foo is defined. So after the clone, it doesn't extrapolate to x.foo being undefined. Even if the generic parameter is const. See here - let me know if/what I'm missing.
Description
TreeBeta.clone()does not currently infer its output parameter from its input parameter.This means that callers must explicitly provide the generic parameter in order to get strong typing of the cloned object.
This PR updates the generic typing to infer the output type automatically.
This may be a breaking change for callers who explicitly provided the generic parameter and must now delete it, as noted in the changeset.