Skip to content

Commit

Permalink
tree: Remove schema aware typing from flex tree schema (#22293)
Browse files Browse the repository at this point in the history
## Description

Remove schema aware typing from flex tree schema

## Reviewer Guidance

The review process is outlined on [this wiki
page](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines).
  • Loading branch information
CraigMacomber authored Aug 22, 2024
1 parent 6ecca8f commit 1106d0b
Show file tree
Hide file tree
Showing 15 changed files with 80 additions and 254 deletions.
5 changes: 4 additions & 1 deletion packages/dds/tree/src/domains/json/jsonCursor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
keyAsDetachedField,
mapCursorField,
mapCursorFields,
type TreeNodeSchemaIdentifier,
} from "../../core/index.js";
import {
type CursorAdapter,
Expand All @@ -30,7 +31,7 @@ const adapter: CursorAdapter<JsonCompatible> = {
node !== null && typeof node === "object"
? undefined // arrays and objects have no defined value
: node, // null, boolean, numbers, and strings are their own values
type: (node: JsonCompatible) => {
type: (node: JsonCompatible): TreeNodeSchemaIdentifier => {
const type = typeof node;

switch (type) {
Expand All @@ -44,6 +45,8 @@ const adapter: CursorAdapter<JsonCompatible> = {
if (node === null) {
return leaf.null.name;
} else if (isReadonlyArray(node)) {
// Linter seems wrong here.
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
return jsonArray.name;
} else {
return jsonObject.name;
Expand Down
11 changes: 8 additions & 3 deletions packages/dds/tree/src/domains/json/jsonDomainSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
FieldKinds,
type FlexAllowedTypes,
FlexFieldSchema,
type FlexTreeNodeSchema,
SchemaBuilderInternal,
} from "../../feature-libraries/index.js";
import type { requireAssignableTo } from "../../util/index.js";
Expand All @@ -28,19 +29,23 @@ const jsonPrimitives = [...leaf.primitives, leaf.null] as const;
/**
* Types allowed as roots of Json content.
*/
export const jsonRoot = [() => jsonObject, () => jsonArray, ...jsonPrimitives] as const;
export const jsonRoot: FlexAllowedTypes = [
(): FlexTreeNodeSchema => jsonObject,
(): FlexTreeNodeSchema => jsonArray,
...jsonPrimitives,
];

{
// Recursive objects don't get this type checking automatically, so confirm it
type _check = requireAssignableTo<typeof jsonRoot, FlexAllowedTypes>;
}

export const jsonObject = builder.mapRecursive(
export const jsonObject = builder.map(
"object",
FlexFieldSchema.createUnsafe(FieldKinds.optional, jsonRoot),
);

export const jsonArray = builder.objectRecursive("array", {
export const jsonArray = builder.object("array", {
[EmptyKey]: FlexFieldSchema.createUnsafe(FieldKinds.sequence, jsonRoot),
});

Expand Down
27 changes: 3 additions & 24 deletions packages/dds/tree/src/domains/schemaBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,9 @@ import {
type FlexFieldKind,
type FlexFieldSchema,
type FlexImplicitAllowedTypes,
type FlexImplicitFieldSchema,
type FlexObjectNodeSchema,
type NormalizeAllowedTypes,
SchemaBuilderBase,
type SchemaBuilderOptions,
type Unenforced,
} from "../feature-libraries/index.js";
import type { RestrictiveReadonlyRecord } from "../util/index.js";

import { leaf } from "./leafDomain.js";

Expand All @@ -41,28 +36,14 @@ import { leaf } from "./leafDomain.js";
* @sealed
* @deprecated Users of this class should either use {@link SchemaBuilderBase} and explicitly work with {@link FlexFieldSchema}, or use SchemaFactory and work at its higher level of abstraction.
*/
export class SchemaBuilder<
TScope extends string = string,
TName extends string | number = string,
> extends SchemaBuilderBase<TScope, typeof FieldKinds.required, TName> {
public constructor(options: SchemaBuilderOptions<TScope>) {
export class SchemaBuilder extends SchemaBuilderBase<string, typeof FieldKinds.required> {
public constructor(options: SchemaBuilderOptions) {
super(FieldKinds.required, {
...options,
libraries: [...(options.libraries ?? []), leaf.library],
});
}

// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
public override objectRecursive<
const Name extends TName,
const T extends Unenforced<RestrictiveReadonlyRecord<string, FlexImplicitFieldSchema>>,
>(name: Name, t: T) {
return this.object(
name,
t as unknown as RestrictiveReadonlyRecord<string, FlexImplicitFieldSchema>,
) as unknown as FlexObjectNodeSchema<`${TScope}.${Name}`, T>;
}

/**
* Define a schema for an {@link FieldKinds.optional|optional field}.
* @remarks
Expand Down Expand Up @@ -146,8 +127,6 @@ export class SchemaBuilder<
* Returns a wrapper around SchemaBuilder.field for a specific FieldKind.
*/
function fieldHelper<Kind extends FlexFieldKind>(kind: Kind) {
return <const T extends FlexImplicitAllowedTypes>(
allowedTypes: T,
): FlexFieldSchema<Kind, NormalizeAllowedTypes<T>> =>
return (allowedTypes: FlexImplicitAllowedTypes): FlexFieldSchema<Kind> =>
SchemaBuilder.field(kind, allowedTypes);
}
38 changes: 0 additions & 38 deletions packages/dds/tree/src/domains/testRecursiveDomain.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ export interface FlexTreeLeafNode<in out TSchema extends LeafNodeSchema> extends
/**
* Value stored on this node.
*/
readonly value: TreeValue<TSchema["info"]>;
readonly value: TreeValue;
}

// #endregion
Expand Down
5 changes: 1 addition & 4 deletions packages/dds/tree/src/feature-libraries/schemaBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,7 @@ export class SchemaBuilderInternal<
* TODO: Maybe ban undefined from allowed values here.
* TODO: Decide and document how unwrapping works for non-primitive terminals.
*/
public leaf<Name extends string, const T extends ValueSchema>(
name: Name,
t: T,
): LeafNodeSchema<`${TScope}.${Name}`, T> {
public leaf(name: string, t: ValueSchema): LeafNodeSchema {
const schema = LeafNodeSchema.create(this, this.scoped(name), t);
this.addNodeSchema(schema);
return schema;
Expand Down
102 changes: 17 additions & 85 deletions packages/dds/tree/src/feature-libraries/schemaBuilderBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { assert } from "@fluidframework/core-utils/internal";
import type { Adapters, TreeNodeSchemaIdentifier } from "../core/index.js";
import {
type Assume,
brand,
type RestrictiveReadonlyRecord,
transformObjectMap,
} from "../util/index.js";
Expand All @@ -18,17 +19,16 @@ import {
Any,
type FlexAllowedTypes,
FlexFieldSchema,
type FlexList,
type FlexMapFieldSchema,
FlexMapNodeSchema,
type FlexObjectNodeFields,
FlexObjectNodeSchema,
type FlexTreeNodeSchema,
type FlexTreeSchema,
type SchemaCollection,
type SchemaLibraryData,
type SchemaLintConfiguration,
TreeNodeSchemaBase,
type Unenforced,
aggregateSchemaLibraries,
schemaLintDefault,
} from "./typed-schema/index.js";
Expand Down Expand Up @@ -78,11 +78,7 @@ export interface SchemaBuilderOptions<TScope extends string = string> {
* or bake in any defaults that might have compatibility implications.
* All use of such implicit defaults is done by subclasses, which thus get versioning implications.
*/
export class SchemaBuilderBase<
TScope extends string,
TDefaultKind extends FlexFieldKind,
TName extends number | string = string,
> {
export class SchemaBuilderBase<TScope extends string, TDefaultKind extends FlexFieldKind> {
private readonly lintConfiguration: SchemaLintConfiguration;
private readonly libraries: Set<SchemaLibraryData>;
private finalized: boolean = false;
Expand Down Expand Up @@ -113,10 +109,8 @@ export class SchemaBuilderBase<
this.scope = options.scope;
}

protected scoped<Name extends TName>(
name: Name,
): TreeNodeSchemaIdentifier<`${TScope}.${Name}`> {
return `${this.scope}.${name}` as TreeNodeSchemaIdentifier<`${TScope}.${Name}`>;
protected scoped(name: string): TreeNodeSchemaIdentifier {
return brand(`${this.scope}.${name}`);
}

/**
Expand Down Expand Up @@ -197,75 +191,31 @@ export class SchemaBuilderBase<
*
* The name must be unique among all TreeNodeSchema in the the document schema.
*/
public object<
const Name extends TName,
const T extends RestrictiveReadonlyRecord<string, FlexImplicitFieldSchema>,
>(
name: Name,
t: T,
): FlexObjectNodeSchema<
`${TScope}.${Name}`,
{ [key in keyof T]: NormalizeField<T[key], TDefaultKind> }
> {
public object(
name: string,
t: RestrictiveReadonlyRecord<string, FlexImplicitFieldSchema>,
): FlexObjectNodeSchema {
const schema = FlexObjectNodeSchema.create(
this,
this.scoped(name),
transformObjectMap(t, (field): FlexFieldSchema => this.normalizeField(field)) as {
[key in keyof T]: NormalizeField<T[key], TDefaultKind>;
},
transformObjectMap(
t,
(field): FlexFieldSchema => this.normalizeField(field),
) as FlexObjectNodeFields,
);
this.addNodeSchema(schema);
return schema;
}

/**
* Same as `object` but with less type safety and works for recursive objects.
* Reduced type safety is a side effect of a workaround for a TypeScript limitation.
*
* See {@link Unenforced} for details.
*
* TODO: Make this work with ImplicitFieldSchema.
*/
public objectRecursive<
const Name extends TName,
const T extends Unenforced<RestrictiveReadonlyRecord<string, FlexImplicitFieldSchema>>,
>(name: Name, t: T): FlexObjectNodeSchema<`${TScope}.${Name}`, T> {
return this.object(
name,
t as unknown as RestrictiveReadonlyRecord<string, FlexImplicitFieldSchema>,
) as unknown as FlexObjectNodeSchema<`${TScope}.${Name}`, T>;
}

/**
* Define (and add to this library) a {@link FlexMapNodeSchema}.
*/
public map<Name extends TName, const T extends FlexMapFieldSchema>(
name: Name,
fieldSchema: T,
): FlexMapNodeSchema<`${TScope}.${Name}`, T> {
public map(name: string, fieldSchema: FlexMapFieldSchema): FlexMapNodeSchema {
const schema = FlexMapNodeSchema.create(this, this.scoped(name), fieldSchema);
this.addNodeSchema(schema);
return schema;
}

/**
* Same as `map` but with less type safety and works for recursive objects.
* Reduced type safety is a side effect of a workaround for a TypeScript limitation.
*
* See {@link Unenforced} for details.
*
* TODO: Make this work with ImplicitFieldSchema.
*/
public mapRecursive<Name extends TName, const T extends Unenforced<FlexMapFieldSchema>>(
name: Name,
t: T,
): FlexMapNodeSchema<`${TScope}.${Name}`, T> {
return this.map(name, t as FlexMapFieldSchema) as FlexMapNodeSchema<
`${TScope}.${Name}`,
T
>;
}

/**
* Define a {@link FlexFieldSchema}.
*
Expand All @@ -280,31 +230,13 @@ export class SchemaBuilderBase<
* If a solution to TreeFieldSchema not being able to have extends clauses gets found,
* consider just having users do `new TreeFieldSchema` instead?
*/
public static field<Kind extends FlexFieldKind, T extends FlexImplicitAllowedTypes>(
public static field<Kind extends FlexFieldKind>(
kind: Kind,
allowedTypes: T,
): FlexFieldSchema<Kind, NormalizeAllowedTypes<T>> {
allowedTypes: FlexImplicitAllowedTypes,
): FlexFieldSchema<Kind> {
return FlexFieldSchema.create(kind, normalizeAllowedTypes(allowedTypes));
}

/**
* Define a schema for a field.
* Same as {@link SchemaBuilderBase.field} but is less type safe and supports recursive types.
* This API is less safe to work around a [limitation of TypeScript](https://github.com/microsoft/TypeScript/issues/55758).
*
* T must extends `AllowedTypes`: This cannot be enforced via TypeScript since such an "extends" clauses cause recursive types to error with:
* "'theSchema' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer."
*
* TODO: Try and find a way to provide a more specific type without triggering the above error.
*/
public static fieldRecursive<
Kind extends FlexFieldKind,
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-arguments
T extends FlexList<Unenforced<FlexTreeNodeSchema>>,
>(kind: Kind, ...allowedTypes: T): FlexFieldSchema<Kind, T> {
return FlexFieldSchema.createUnsafe(kind, allowedTypes);
}

/**
* Normalizes an {@link FlexImplicitFieldSchema} into a {@link FlexFieldSchema} using this schema builder's `defaultKind`.
*/
Expand Down
Loading

0 comments on commit 1106d0b

Please sign in to comment.