Skip to content

Commit

Permalink
tree: Refer to property keys, not view keys (#22366)
Browse files Browse the repository at this point in the history
## Description

Clarify docs and comments to refer to property keys not view keys.

Changes split out from
#22229 for separate
review.
  • Loading branch information
CraigMacomber authored Sep 3, 2024
1 parent 651cb19 commit f4da8fc
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 55 deletions.
34 changes: 17 additions & 17 deletions packages/dds/tree/src/simple-tree/api/treeNodeApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,12 @@ export const treeNodeApi: TreeNodeApi = {
}

// The flex-domain strictly operates in terms of "stored keys".
// To find the associated developer-facing "view key", we need to look up the field associated with
// the stored key from the flex-domain, and get view key its simple-domain counterpart was created with.
// To find the associated developer-facing "property key", we need to look up the field associated with
// the stored key from the flex-domain, and get property key its simple-domain counterpart was created with.
const storedKey = getStoredKey(node);
const parentSchema = treeNodeApi.schema(parent);
const viewKey = getViewKeyFromStoredKey(parentSchema, storedKey);
return viewKey;
const propertyKey = getPropertyKeyFromStoredKey(parentSchema, storedKey);
return propertyKey;
},
on<K extends keyof TreeChangeEvents>(
node: TreeNode,
Expand Down Expand Up @@ -257,7 +257,7 @@ export function tryGetSchema(value: unknown): undefined | TreeNodeSchema {
return booleanSchema;
case "object": {
if (isTreeNode(value)) {
// This case could be optimized, for example by placing the simple schema in a symbol on tree nodes.
// TODO: This case could be optimized, for example by placing the simple schema in a symbol on tree nodes.
return tryGetTreeNodeSchema(value);
}
if (value === null) {
Expand All @@ -277,7 +277,7 @@ export function tryGetSchema(value: unknown): undefined | TreeNodeSchema {
*/
function getStoredKey(node: TreeNode): string | number {
// Note: the flex domain strictly works with "stored keys", and knows nothing about the developer-facing
// "view keys".
// "property keys".
const parentField = getOrCreateInnerNode(node).parentField;
if (parentField.parent.schema.kind.multiplicity === Multiplicity.Sequence) {
// The parent of `node` is an array node
Expand All @@ -289,33 +289,33 @@ function getStoredKey(node: TreeNode): string | number {
}

/**
* Given a node schema, gets the view key corresponding with the provided {@link FieldProps.key | stored key}.
* Given a node schema, gets the property key corresponding with the provided {@link FieldProps.key | stored key}.
*/
function getViewKeyFromStoredKey(
function getPropertyKeyFromStoredKey(
schema: TreeNodeSchema,
storedKey: string | number,
): string | number {
// Only object nodes have the concept of a "stored key", differentiated from the developer-facing "view key".
// For any other kind of node, the stored key and the view key are the same.
// Only object nodes have the concept of a "stored key", differentiated from the developer-facing "property key".
// For any other kind of node, the stored key and the property key are the same.
if (schema.kind !== NodeKind.Object) {
return storedKey;
}

const fields = schema.info as Record<string, ImplicitFieldSchema>;

// Invariants:
// - The set of all view keys under an object must be unique.
// - The set of all stored keys (including those implicitly created from view keys) must be unique.
// To find the view key associated with the provided stored key, first check for any stored key matches (which are optionally populated).
// If we don't find any, then search for a matching view key.
for (const [viewKey, fieldSchema] of Object.entries(fields)) {
// - The set of all property keys under an object must be unique.
// - The set of all stored keys (including those implicitly created from property keys) must be unique.
// To find the property key associated with the provided stored key, first check for any stored key matches (which are optionally populated).
// If we don't find any, then search for a matching property key.
for (const [propertyKey, fieldSchema] of Object.entries(fields)) {
if (fieldSchema instanceof FieldSchema && fieldSchema.props?.key === storedKey) {
return viewKey;
return propertyKey;
}
}

if (fields[storedKey] === undefined) {
fail("Existing stored key should always map to a view key");
fail("Existing stored key should always map to a property key");
}

return storedKey;
Expand Down
56 changes: 29 additions & 27 deletions packages/dds/tree/src/simple-tree/objectNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,10 @@ export type InsertableObjectFromSchemaRecord<
>;

/**
* Maps from simple field keys ("view" keys) to information about the field.
* Maps from simple field keys ("property" keys) to information about the field.
*
* @remarks
* A missing entry for a given view key indicates that no such field exists.
* A missing entry for a given property key indicates that no such field exists.
* Keys with symbols are currently never used, but allowed to make lookups on non-field things
* (returning undefined) easier.
*/
Expand All @@ -133,13 +133,13 @@ export type SimpleKeyMap = ReadonlyMap<
>;

/**
* Caches the mappings from view keys to stored keys for the provided object field schemas in {@link simpleKeyToFlexKeyCache}.
* Caches the mappings from property keys to stored keys for the provided object field schemas in {@link simpleKeyToFlexKeyCache}.
*/
function createFlexKeyMapping(fields: Record<string, ImplicitFieldSchema>): SimpleKeyMap {
const keyMap: Map<string | symbol, { storedKey: FieldKey; schema: FieldSchema }> = new Map();
for (const [viewKey, fieldSchema] of Object.entries(fields)) {
const storedKey = getStoredKey(viewKey, fieldSchema);
keyMap.set(viewKey, { storedKey, schema: normalizeFieldSchema(fieldSchema) });
for (const [propertyKey, fieldSchema] of Object.entries(fields)) {
const storedKey = getStoredKey(propertyKey, fieldSchema);
keyMap.set(propertyKey, { storedKey, schema: normalizeFieldSchema(fieldSchema) });
}

return keyMap;
Expand All @@ -166,8 +166,8 @@ function createProxyHandler(
// TODO: Although the target is an object literal, it's still worthwhile to try experimenting with
// a dispatch object to see if it improves performance.
const handler: ProxyHandler<TreeNode> = {
get(target, viewKey, proxy): unknown {
const fieldInfo = flexKeyMap.get(viewKey);
get(target, propertyKey, proxy): unknown {
const fieldInfo = flexKeyMap.get(propertyKey);

if (fieldInfo !== undefined) {
const flexNode = getOrCreateInnerNode(proxy);
Expand All @@ -189,13 +189,15 @@ function createProxyHandler(
}

// Pass the proxy as the receiver here, so that any methods on the prototype receive `proxy` as `this`.
return Reflect.get(target, viewKey, proxy);
return Reflect.get(target, propertyKey, proxy);
},
set(target, viewKey, value: InsertableContent | undefined, proxy) {
const fieldInfo = flexKeyMap.get(viewKey);
set(target, propertyKey, value: InsertableContent | undefined, proxy) {
const fieldInfo = flexKeyMap.get(propertyKey);
if (fieldInfo === undefined) {
// Pass the proxy as the receiver here, so that setters on the prototype receive `proxy` as `this`.
return allowAdditionalProperties ? Reflect.set(target, viewKey, value, proxy) : false;
return allowAdditionalProperties
? Reflect.set(target, propertyKey, value, proxy)
: false;
}

setField(
Expand All @@ -205,16 +207,16 @@ function createProxyHandler(
);
return true;
},
deleteProperty(target, viewKey): boolean {
deleteProperty(target, propertyKey): boolean {
// TODO: supporting delete when it makes sense (custom local fields, and optional field) could be added as a feature in the future.
throw new UsageError(
`Object nodes do not support the delete operator. Optional fields can be assigned to undefined instead.`,
);
},
has: (target, viewKey) => {
has: (target, propertyKey) => {
return (
flexKeyMap.has(viewKey) ||
(allowAdditionalProperties ? Reflect.has(target, viewKey) : false)
flexKeyMap.has(propertyKey) ||
(allowAdditionalProperties ? Reflect.has(target, propertyKey) : false)
);
},
ownKeys: (target) => {
Expand All @@ -223,12 +225,12 @@ function createProxyHandler(
...(allowAdditionalProperties ? Reflect.ownKeys(target) : []),
];
},
getOwnPropertyDescriptor: (target, viewKey) => {
const fieldInfo = flexKeyMap.get(viewKey);
getOwnPropertyDescriptor: (target, propertyKey) => {
const fieldInfo = flexKeyMap.get(propertyKey);

if (fieldInfo === undefined) {
return allowAdditionalProperties
? Reflect.getOwnPropertyDescriptor(target, viewKey)
? Reflect.getOwnPropertyDescriptor(target, propertyKey)
: undefined;
}

Expand Down Expand Up @@ -309,11 +311,11 @@ export function objectSchema<
info: T,
implicitlyConstructable: ImplicitlyConstructable,
): ObjectNodeSchema<TName, T, ImplicitlyConstructable> & ObjectNodeSchemaInternalData {
// Ensure no collisions between final set of view keys, and final set of stored keys (including those
// implicitly derived from view keys)
// Ensure no collisions between final set of property keys, and final set of stored keys (including those
// implicitly derived from property keys)
assertUniqueKeys(identifier, info);

// Performance optimization: cache view key => stored key and schema.
// Performance optimization: cache property key => stored key and schema.
const flexKeyMap: SimpleKeyMap = createFlexKeyMapping(info);

let handler: ProxyHandler<object>;
Expand Down Expand Up @@ -433,8 +435,8 @@ export function objectSchema<
const targetToProxy: WeakMap<object, TreeNode> = new WeakMap();

/**
* Ensures that the set of view keys in the schema is unique.
* Also ensure that the final set of stored keys (including those implicitly derived from view keys) is unique.
* Ensures that the set of property keys in the schema is unique.
* Also ensure that the final set of stored keys (including those implicitly derived from property keys) is unique.
* @throws Throws a `UsageError` if either of the key uniqueness invariants is violated.
*/
function assertUniqueKeys<
Expand All @@ -457,10 +459,10 @@ function assertUniqueKeys<
}

// Verify that there are no duplicates among the derived
// (including those implicitly derived from view keys) stored keys.
// (including those implicitly derived from property keys) stored keys.
const derivedStoredKeys = new Set<string>();
for (const [viewKey, schema] of Object.entries(fields)) {
const storedKey = getStoredKey(viewKey, schema);
for (const [propertyKey, schema] of Object.entries(fields)) {
const storedKey = getStoredKey(propertyKey, schema);
if (derivedStoredKeys.has(storedKey)) {
throw new UsageError(
`Stored key "${storedKey}" in schema "${schemaName}" conflicts with a property key of the same name, which is not overridden by a stored key. The final set of stored keys in an object schema must be unique.`,
Expand Down
8 changes: 4 additions & 4 deletions packages/dds/tree/src/simple-tree/schemaTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,15 @@ export enum FieldKind {
}

/**
* Maps from a view key to its corresponding {@link FieldProps.key | stored key} for the provided
* Maps from a property key to its corresponding {@link FieldProps.key | stored key} for the provided
* {@link ImplicitFieldSchema | field schema}.
*
* @remarks
* If an explicit stored key was specified in the schema, it will be used.
* Otherwise, the stored key is the same as the view key.
* Otherwise, the stored key is the same as the property key.
*/
export function getStoredKey(viewKey: string, fieldSchema: ImplicitFieldSchema): FieldKey {
return brand(getExplicitStoredKey(fieldSchema) ?? viewKey);
export function getStoredKey(propertyKey: string, fieldSchema: ImplicitFieldSchema): FieldKey {
return brand(getExplicitStoredKey(fieldSchema) ?? propertyKey);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/dds/tree/src/simple-tree/simpleSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export interface SimpleNodeSchemaBase<TNodeKind extends NodeKind> {
*/
export interface SimpleObjectNodeSchema extends SimpleNodeSchemaBase<NodeKind.Object> {
/**
* Schemas for each of the object's fields, keyed off of schema's view keys.
* Schemas for each of the object's fields, keyed off of schema's property keys.
*/
readonly fields: Record<string, SimpleFieldSchema>;
}
Expand Down
6 changes: 3 additions & 3 deletions packages/dds/tree/src/simple-tree/toFlexSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,10 @@ export function convertNodeSchema(
case NodeKind.Object: {
const info = schema.info as Record<string, ImplicitFieldSchema>;
const fields: Record<string, FlexFieldSchema> = Object.create(null);
for (const [viewKey, implicitFieldSchema] of Object.entries(info)) {
for (const [propertyKey, implicitFieldSchema] of Object.entries(info)) {
// If a `stored key` was provided, use it as the key in the flex schema.
// Otherwise, use the view key.
const flexKey = getStoredKey(viewKey, implicitFieldSchema);
// Otherwise, use the property key.
const flexKey = getStoredKey(propertyKey, implicitFieldSchema);

// This code has to be careful to avoid assigning to __proto__ or similar built-in fields.
Object.defineProperty(fields, flexKey, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ describe("schemaFactory", () => {
);
});

it("Stored key collides with view key", () => {
it("Stored key collides with property key", () => {
const schema = new SchemaFactory("com.example");
assert.throws(
() =>
Expand All @@ -342,7 +342,7 @@ describe("schemaFactory", () => {

// This is a somewhat neurotic test case, and likely not something we would expect a user to do.
// But just in case, we should ensure it is handled correctly.
it("Stored key / view key swap", () => {
it("Stored key / property key swap", () => {
const schema = new SchemaFactory("com.example");
assert.doesNotThrow(() =>
schema.object("Object", {
Expand All @@ -352,7 +352,7 @@ describe("schemaFactory", () => {
);
});

it("Explicit stored key === view key", () => {
it("Explicit stored key === property key", () => {
const schema = new SchemaFactory("com.example");
assert.doesNotThrow(() =>
schema.object("Object", {
Expand Down

0 comments on commit f4da8fc

Please sign in to comment.