diff --git a/packages/firecms_core/src/types/properties.ts b/packages/firecms_core/src/types/properties.ts index 6496ad275..08cb648ba 100644 --- a/packages/firecms_core/src/types/properties.ts +++ b/packages/firecms_core/src/types/properties.ts @@ -562,6 +562,22 @@ export type PropertyBuilderProps = any> = * Current value of this property */ propertyValue?: any; + /** + * Path of this property within the entity, in dot notation, e.g. + * `my_map.my_field` or `my_array.0.my_field`. + * Useful to look up sibling values in `values` with `getValueInPath`. + * + * When a property builder is used as the `of` prop of an array property, + * it is resolved twice: once per existing element, with an index + * qualified key (`my_array.0`), and once as the template for the element + * shape, with the array's own key (`my_array`). In the latter case + * `propertyValue` is the whole array, since the template is not bound to + * any element. + * + * It may be undefined if the property is resolved outside the context of + * an entity. + */ + propertyKey?: string; /** * Index of this property (only for arrays) */ @@ -592,6 +608,7 @@ export type PropertyBuilder({ ignoreMissingFields, ...props }); + // `of` describes the shape of any element of the array, not one + // specific element, so there is no index to qualify the key with. + // We pass the array's own key, mirroring what the `oneOf` branch + // below does for `oneOf.properties`. Without this, a property + // builder used as `of` (or nested inside it) would be invoked with + // `propertyKey: undefined`, while the per-index resolutions above + // receive `${propertyKey}.${index}`. const ofProperty = resolveProperty({ + propertyKey, propertyOrBuilder: of, ignoreMissingFields, ...props diff --git a/packages/firecms_core/test/resolutions.test.ts b/packages/firecms_core/test/resolutions.test.ts index 9cdfbcabe..67c59100e 100644 --- a/packages/firecms_core/test/resolutions.test.ts +++ b/packages/firecms_core/test/resolutions.test.ts @@ -1,5 +1,6 @@ import { describe, test } from "@jest/globals"; import { buildCollection, buildProperty, resolveCollection } from "../src/util"; +import { PropertyBuilderProps } from "../src/types"; import * as util from "util"; const testCollection = buildCollection({ @@ -162,3 +163,311 @@ describe("resolutions", () => { expect(resolved.defaultSize).toBe("s"); }); }); + +/** + * Regression tests for https://github.com/firecmsco/firecms/issues/659 + * + * `propertyKey` must be handed to every property builder that gets resolved, + * for every array resolution path (`of` as a tuple, `of` as a single property + * or builder, and `oneOf`). Builders rely on it to locate sibling values with + * dot notation (e.g. `example.0.parent` from `example.0.child`). + */ +describe("resolutions - propertyKey passed to property builders", () => { + + /** + * Records the props every builder invocation receives, so we can assert on + * the exact `propertyKey`/`propertyValue` pairs and their order. + */ + function recordingBuilder(calls: { propertyKey?: string, propertyValue?: any, index?: number }[], + result: any = { + name: "Child", + dataType: "string" + }) { + // Destructuring `propertyKey` out of the typed `PropertyBuilderProps` + // also asserts, at compile time, that it is part of the public contract. + return buildProperty(({ + propertyKey, + propertyValue, + index + }: PropertyBuilderProps) => { + calls.push({ + propertyKey, + propertyValue, + index + }); + return result; + }); + } + + function resolve(properties: any, values: any) { + return resolveCollection({ + collection: buildCollection({ + id: "test_659", + path: "test_659", + name: "Test 659", + properties + }), + path: "test_659", + values, + authController: {} as any + }); + } + + test("a builder nested in the map of an array `of` always receives a defined propertyKey", () => { + + // This is the exact shape reported in #659: a builder inside a map, + // inside an array, that needs to find its sibling `parent` value. + const calls: { propertyKey?: string, propertyValue?: any }[] = []; + + const resolved = resolve({ + example: { + name: "Example", + dataType: "array", + of: { + name: "Entry", + dataType: "map", + properties: { + parent: { + name: "Parent", + dataType: "string" + }, + child: recordingBuilder(calls) + } + } + } + }, { + example: [ + { parent: "first" }, + { parent: "second" } + ] + }); + + // The bug: the `of` template resolution used to invoke the builder with + // `propertyKey: undefined`. + expect(calls.map(c => c.propertyKey)).not.toContain(undefined); + + // Two per-index resolutions (one per array element), then the single + // `of` template resolution. + expect(calls.map(c => c.propertyKey)).toEqual([ + "example.0.child", + "example.1.child", + "example.child" + ]); + + // A builder can therefore always derive its siblings' paths. + const siblingPaths = calls.map(c => [...c.propertyKey!.split(".").slice(0, -1), "parent"].join(".")); + expect(siblingPaths).toEqual(["example.0.parent", "example.1.parent", "example.parent"]); + + // The resolved collection still exposes both the `of` template and the + // per-index resolved properties. + const exampleProperty = resolved.properties.example as any; + expect(exampleProperty.of.properties.child.dataType).toEqual("string"); + expect(exampleProperty.resolvedProperties).toHaveLength(2); + expect(exampleProperty.resolvedProperties[0].properties.child.dataType).toEqual("string"); + }); + + test("a builder used directly as an array `of` receives a defined propertyKey", () => { + + const calls: { propertyKey?: string, propertyValue?: any, index?: number }[] = []; + + resolve({ + example: { + name: "Example", + dataType: "array", + of: recordingBuilder(calls) + } + }, { + example: ["first", "second"] + }); + + expect(calls.map(c => c.propertyKey)).not.toContain(undefined); + expect(calls).toEqual([ + { + propertyKey: "example.0", + propertyValue: "first", + index: 0 + }, + { + propertyKey: "example.1", + propertyValue: "second", + index: 1 + }, + // The `of` template is not tied to any element, so it resolves + // against the array property's own key. `propertyValue` is + // consequently the whole array. + { + propertyKey: "example", + propertyValue: ["first", "second"], + index: undefined + } + ]); + }); + + test("the `of` template builder is still resolved when the array has no value", () => { + + const calls: { propertyKey?: string, propertyValue?: any }[] = []; + + resolve({ + example: { + name: "Example", + dataType: "array", + of: recordingBuilder(calls) + } + }, {}); + + // No elements, so only the `of` template resolution happens. + expect(calls).toEqual([ + { + propertyKey: "example", + propertyValue: undefined, + index: undefined + } + ]); + }); + + test("a tuple `of` passes index qualified property keys", () => { + + const calls: { propertyKey?: string, propertyValue?: any, index?: number }[] = []; + + resolve({ + example: { + name: "Example", + dataType: "array", + of: [ + { + name: "Plain", + dataType: "string" + }, + recordingBuilder(calls) + ] + } + }, { + example: ["plain value", "builder value"] + }); + + expect(calls).toEqual([ + { + propertyKey: "example.1", + propertyValue: "builder value", + index: 1 + } + ]); + }); + + test("`oneOf` passes index qualified property keys per element and the array key for the templates", () => { + + const calls: { propertyKey?: string, propertyValue?: any }[] = []; + + resolve({ + example: { + name: "Example", + dataType: "array", + oneOf: { + properties: { + alpha: recordingBuilder(calls), + beta: { + name: "Beta", + dataType: "number" + } + } + } + } + }, { + example: [ + { + type: "alpha", + value: "first" + }, + { + type: "beta", + value: 2 + } + ] + }); + + expect(calls.map(c => c.propertyKey)).toEqual([ + // Per-element resolution of the `alpha` branch. + "example.0", + // Template resolution of `oneOf.properties`, based on the array key. + "example.alpha" + ]); + }); + + test("resolving non builder properties is unaffected", () => { + + const resolved = resolve({ + example: { + name: "Example", + dataType: "array", + of: { + name: "Entry", + dataType: "map", + properties: { + parent: { + name: "Parent", + dataType: "string" + } + } + } + } + }, { + example: [{ parent: "first" }] + }); + + const expectedOf = { + name: "Entry", + dataType: "map", + resolved: true, + fromBuilder: false, + properties: { + parent: { + name: "Parent", + dataType: "string", + resolved: true, + fromBuilder: false + } + } + }; + + expect(resolved.properties.example).toEqual({ + name: "Example", + dataType: "array", + resolved: true, + fromBuilder: false, + of: expectedOf, + resolvedProperties: [expectedOf] + }); + }); + + test("a builder resolved as a plain (non array) property still receives its propertyKey", () => { + + const calls: { propertyKey?: string, propertyValue?: any }[] = []; + + resolve({ + plain: recordingBuilder(calls), + nested: { + name: "Nested", + dataType: "map", + properties: { + child: recordingBuilder(calls) + } + } + }, { + plain: "plain value", + nested: { child: "nested value" } + }); + + expect(calls).toEqual([ + { + propertyKey: "plain", + propertyValue: "plain value", + index: undefined + }, + { + propertyKey: "nested.child", + propertyValue: "nested value", + index: undefined + } + ]); + }); +});