Skip to content

Commit

Permalink
fix: enum adapter fixes (#23077)
Browse files Browse the repository at this point in the history
## Description

The alpha enum APIs had some typing issues where they were unioning
things at the wrong levels.

Additionally since their tests predated unhydrated node support, they
didn't make as many nodes as they probably should have.
The tests have been simplified to use unhydrated nodes in a few places
and extended to cover more cases.
More explicit type testing has been added.

This fixes an issue reported in
#23055

See changeset for details.

## Breaking Changes

See changeset.
  • Loading branch information
CraigMacomber authored Nov 14, 2024
1 parent 3aff19a commit cfb6838
Show file tree
Hide file tree
Showing 5 changed files with 304 additions and 57 deletions.
37 changes: 37 additions & 0 deletions .changeset/full-places-know.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
---
"fluid-framework": minor
"@fluidframework/tree": minor
---
---
"section": tree
---

Fix typing bug in `adaptEnum` and `enumFromStrings`

When using the return value from [`adaptEnum`](https://fluidframework.com/docs/api/v2/tree#adaptenum-function) as a function, passing in a value who's type is a union no longer produced an incorrectly typed return value. This has been fixed.

Additionally [`enumFromStrings`](https://fluidframework.com/docs/api/v2/tree#enumfromstrings-function) has improved the typing of its schema, ensuring the returned object's members have sufficiently specific types.
Part of this improvement was fixing the `.schema` property to be a tuple over each of the schema where it was previously a tuple of a single combined schema due to a bug.

One side-effect of these fixes is that narrowing of the `value` field of a node typed from the `.schema` behaves slightly different, such that the node type is now a union instead of it being a single type with a `.value` that is a union.
This means that narrowing based on `.value` property narrows which node type you have, not just the value property.
This mainly matters when matching all cases like the switch statement below:

```typescript
const Mode = enumFromStrings(schema, ["Fun", "Bonus"]);
type Mode = TreeNodeFromImplicitAllowedTypes<typeof Mode.schema>;
const node = new Mode.Bonus() as Mode;

switch (node.value) {
case "Fun": {
assert.fail();
}
case "Bonus": {
// This one runs
break;
}
default:
// Before this change, "node.value" was never here, now "node" is never.
unreachableCase(node);
}
```
18 changes: 9 additions & 9 deletions packages/dds/tree/api-report/tree.alpha.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
```ts

// @alpha
export function adaptEnum<TScope extends string, const TEnum extends Record<string, string | number>>(factory: SchemaFactory<TScope>, members: TEnum): (<TValue extends TEnum[keyof TEnum]>(value: TValue) => TreeNode & {
export function adaptEnum<TScope extends string, const TEnum extends Record<string, string | number>>(factory: SchemaFactory<TScope>, members: TEnum): (<TValue extends TEnum[keyof TEnum]>(value: TValue) => TValue extends unknown ? TreeNode & {
readonly value: TValue;
}) & { readonly [Property in keyof TEnum]: TreeNodeSchemaClass<ScopedSchemaName<TScope, TEnum[Property]>, NodeKind.Object, TreeNode & {
} : never) & { readonly [Property in keyof TEnum]: TreeNodeSchemaClass<ScopedSchemaName<TScope, TEnum[Property]>, NodeKind.Object, TreeNode & {
readonly value: TEnum[Property];
}, Record<string, never>, true, Record<string, never>, undefined>; } & {
readonly schema: UnionToTuple<{ readonly [Property in keyof TEnum]: TreeNodeSchemaClass<ScopedSchemaName<TScope, TEnum[Property]>, NodeKind.Object, TreeNode & {
Expand Down Expand Up @@ -76,14 +76,14 @@ export interface EncodeOptions<TCustom> {
}

// @alpha
export function enumFromStrings<TScope extends string, const Members extends readonly string[]>(factory: SchemaFactory<TScope>, members: Members): (<TValue extends Members[number]>(value: TValue) => TreeNode & {
export function enumFromStrings<TScope extends string, const Members extends readonly string[]>(factory: SchemaFactory<TScope>, members: Members): (<TValue extends Members[number]>(value: TValue) => TValue extends unknown ? TreeNode & {
readonly value: TValue;
}) & Record<Members[number], TreeNodeSchemaClass<ScopedSchemaName<TScope, Members[number]>, NodeKind.Object, TreeNode & {
readonly value: Members[number];
}, Record<string, never>, true, Record<string, never>, undefined>> & {
readonly schema: UnionToTuple<Record<Members[number], TreeNodeSchemaClass<ScopedSchemaName<TScope, Members[number]>, NodeKind.Object, TreeNode & {
readonly value: Members[number];
}, Record<string, never>, true, Record<string, never>, undefined>>[Members[number]]>;
} : never) & { [Index in Extract<keyof Members, `${number}`> extends `${infer N extends number}` ? N : never as Members[Index]]: TreeNodeSchemaClass<ScopedSchemaName<TScope, Members[Index]>, NodeKind.Object, TreeNode & {
readonly value: Members[Index];
}, Record<string, never>, true, Record<string, never>, undefined>; } & {
readonly schema: UnionToTuple<Members[number] extends unknown ? { [Index in Extract<keyof Members, `${number}`> extends `${infer N extends number}` ? N : never as Members[Index]]: TreeNodeSchemaClass<ScopedSchemaName<TScope, Members[Index]>, NodeKind.Object, TreeNode & {
readonly value: Members[Index];
}, Record<string, never>, true, Record<string, never>, undefined>; }[Members[number]] : never>;
};

// @public
Expand Down
46 changes: 29 additions & 17 deletions packages/dds/tree/src/simple-tree/api/schemaCreationUtilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ export function singletonSchema<TScope extends string, TName extends string | nu
/**
* Converts an enum into a collection of schema which can be used in a union.
* @remarks
* Currently only supports `string` enums.
* The string value of the enum is used as the name of the schema: callers must ensure that it is stable and unique.
* Numeric enums values have the value implicitly converted into a string.
* Consider making a dedicated schema factory with a nested scope to avoid the enum members colliding with other schema.
* @example
* ```typescript
Expand All @@ -87,7 +87,7 @@ export function singletonSchema<TScope extends string, TName extends string | nu
* // Define the schema for each member of the enum using a nested scope to group them together.
* const ModeNodes = adaptEnum(new SchemaFactory(`${schemaFactory.scope}.Mode`), Mode);
* // Defined the types of the nodes which correspond to this the schema.
* type ModeNodes = NodeFromSchema<(typeof ModeNodes.schema)[number]>;
* type ModeNodes = TreeNodeFromImplicitAllowedTypes<(typeof ModeNodes.schema)>;
* // An example schema which has an enum as a child.
* class Parent extends schemaFactory.object("Parent", {
* // adaptEnum's return value has a ".schema" property can be use as an `AllowedTypes` array allowing any of the members of the enum.
Expand All @@ -106,8 +106,6 @@ export function singletonSchema<TScope extends string, TName extends string | nu
* }
* ```
* @privateRemarks
* TODO:
* Extend this to support numeric enums.
* Maybe provide `SchemaFactory.nested` to ease creating nested scopes?
* @see {@link enumFromStrings} for a similar function that works on arrays of strings instead of an enum.
* @alpha
Expand Down Expand Up @@ -139,9 +137,12 @@ export function adaptEnum<

// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
const factoryOut = <TValue extends Values>(value: TValue) => {
return new out[inverse.get(value) ?? fail("missing enum value")]() as NodeFromSchema<
ReturnType<typeof singletonSchema<TScope, TValue>>
>;
return new out[
inverse.get(value) ?? fail("missing enum value")
// "extends unknown" is required here to handle when TValue is an union: each member of the union should be processed independently.
]() as TValue extends unknown
? NodeFromSchema<ReturnType<typeof singletonSchema<TScope, TValue>>>
: never;
};
const out = factoryOut as typeof factoryOut & TOut & { readonly schema: SchemaArray };
for (const [key, value] of Object.entries(members)) {
Expand Down Expand Up @@ -176,7 +177,7 @@ export function adaptEnum<
* ```typescript
* const schemaFactory = new SchemaFactory("com.myApp");
* const Mode = enumFromStrings(schemaFactory, ["Fun", "Cool"]);
* type Mode = NodeFromSchema<(typeof Mode.schema)[number]>;
* type Mode = TreeNodeFromImplicitAllowedTypes<typeof Mode.schema>;
* const nodeFromString: Mode = Mode("Fun");
* const nodeFromSchema: Mode = new Mode.Fun();
*
Expand All @@ -198,21 +199,32 @@ export function enumFromStrings<
throw new UsageError("All members of enums must have distinct names");
}

type TOut = Record<
Members[number],
ReturnType<typeof singletonSchema<TScope, Members[number]>>
>;
// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
const factoryOut = <TValue extends Members[number]>(value: TValue) => {
return new out[value]() as NodeFromSchema<
ReturnType<typeof singletonSchema<TScope, TValue>>
type MembersUnion = Members[number];

// Get all keys of the Members tuple which are numeric strings as union of numbers:
type Indexes = Extract<keyof Members, `${number}`> extends `${infer N extends number}`
? N
: never;

type TOut = {
[Index in Indexes as Members[Index]]: ReturnType<
typeof singletonSchema<TScope, Members[Index] & string>
>;
};

type SchemaArray = UnionToTuple<TOut[Members[number]]>;
// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
const factoryOut = <TValue extends MembersUnion>(value: TValue) => {
// "extends unknown" is required here to handle when TValue is an union: each member of the union should be processed independently.
return new recordOut[value]() as TValue extends unknown
? NodeFromSchema<ReturnType<typeof singletonSchema<TScope, TValue>>>
: never;
};

type SchemaArray = UnionToTuple<MembersUnion extends unknown ? TOut[MembersUnion] : never>;
const schemaArray: TreeNodeSchema[] = [];

const out = factoryOut as typeof factoryOut & TOut & { readonly schema: SchemaArray };
const recordOut = out as Record<MembersUnion, new () => unknown>;
for (const name of members) {
const schema = singletonSchema(factory, name);
schemaArray.push(schema);
Expand Down
Loading

0 comments on commit cfb6838

Please sign in to comment.