diff --git a/.changeset/26306-filter-builder-spec-row-value.md b/.changeset/26306-filter-builder-spec-row-value.md new file mode 100644 index 00000000000..c719cf03435 --- /dev/null +++ b/.changeset/26306-filter-builder-spec-row-value.md @@ -0,0 +1,25 @@ +--- +'@object-ui/components': patch +--- + +fix(components)!: the filter builder's row takes the spec rule's value, so the authored group a `filter-builder` node carries reaches the builder as it is + +Clause-②: yes (narrowing) +Metadata-protocol: none + + + +**BREAKING** type-surface change, compile-time only. `FilterBuilderRow` is exported from `@object-ui/components`. What the builder accepts widens, and what a reader of `row.value` may assume narrows: the member is now optional and admits `null`. Nothing changes at runtime for a reader: the builder already passed both through. + +`FilterBuilderRow.value` is now `ViewFilterRule['value']` from `@objectstack/spec/ui`, taken by reference, plus the builder's own list member `(string | number | boolean)[]`. Before, it was the builder's own `string | number | boolean | (string | number | boolean)[]`, and it was required. Measured against the spec's rule, that refused two shapes an authored row takes: + +- **Value omitted.** The spec's value is optional, because a value-less operator (`is_null`, `is_empty`, …) carries none. The builder's was required, so the authored `FilterGroup` from `@object-ui/types` was not assignable to the builder's group. The member is optional now. +- **Value `null`.** The spec admits it; the builder's member did not. It does now. + +The operator vocabulary needed no change: every spec operator id is a builder id. + +Both cases also fix a field switch. Switching the field of a row with no value used to convert the absent value: on a number column it threw on `.trim()`, and on a text column it came back as the string `"undefined"`. A row with no value, or a `null` one, now keeps it through a field switch. + +The `filter-builder` renderer now hands the authored `value` and `fields` to the builder with no cast. It declares `value` on its props, because a host that binds the node forwards the group it holds. + +**Migration.** Code that reads `row.value` from a `FilterBuilderRow` (a host's `onChange` handler, for instance) handles `undefined` and `null` before using the value, or narrows first with an unset check (`value === undefined || value === null || value === ''`). Code that only writes rows needs no change. diff --git a/scripts/sdui-manifest.sources/components.json b/scripts/sdui-manifest.sources/components.json index f3006fdc5f9..abf8b2d57ee 100644 --- a/scripts/sdui-manifest.sources/components.json +++ b/scripts/sdui-manifest.sources/components.json @@ -1 +1 @@ -{"package":"ui/packages/components","sources":226,"sha256":"cd1d23767a76e6b4dcf2c0d4ca61b10ba27b336eeab990b22bfb0f7d10bb0424","manifestSha256":"1f6e1ccb925a678eaa393f9091dfc7d0285e4e3626e0a5c61a5474687debabb7"} +{"package":"ui/packages/components","sources":226,"sha256":"75ed8c66878430e296c285bb0885304449f091b00da2209889cb99f634ef1690","manifestSha256":"1f6e1ccb925a678eaa393f9091dfc7d0285e4e3626e0a5c61a5474687debabb7"} diff --git a/ui/packages/components/docs/FilterBuilder.md b/ui/packages/components/docs/FilterBuilder.md index efa0ba3fc43..0e2824d9990 100644 --- a/ui/packages/components/docs/FilterBuilder.md +++ b/ui/packages/components/docs/FilterBuilder.md @@ -122,7 +122,8 @@ interface Field { The schema's `value` is `FilterGroup` from `@object-ui/types`, the authored group. The React component's `value` / `onChange` use `FilterBuilderRowGroup` from `@object-ui/components`: the same group, derived from `FilterGroup`, whose rows -are `FilterBuilderRow` (the builder's own operator ids and a required value). +are `FilterBuilderRow` (the builder's own operator ids and the spec rule's value), +so an authored group passes to the component as it is. ```typescript interface FilterBuilderRowGroup { @@ -135,7 +136,9 @@ interface FilterBuilderRow { id: string; // Row identifier (required) field: string; // Field value operator: FilterBuilderOperator; // Operator id (see below) - value: string | number | boolean | (string | number | boolean)[]; // Filter value + value?: ViewFilterRule['value'] | (string | number | boolean)[]; + // `ViewFilterRule` from `@objectstack/spec/ui`: optional (value-less operators + // carry none) and may be null; the builder's own list may also hold booleans } ``` diff --git a/ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx b/ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx index b5cb5dc7b63..35eb5e0df7b 100644 --- a/ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx +++ b/ui/packages/components/src/__tests__/filter-builder-name-authority-9306.test.tsx @@ -20,7 +20,7 @@ * * - `operator` — `FilterBuilderOperator`, the protocol's ids plus the opt-in * `exists` / `notExists` (objectui#9559 ruling B); - * - `value` — required and narrowed to what this builder edits. + * - `value` — the spec rule's value, by reference (#26306). * * Everything else comes from the authority, and the group restates only * `conditions` (to hold this package's row), so its `id` is optional here as it @@ -41,7 +41,7 @@ import { describe, it, expect, vi } from 'vitest'; import React from 'react'; import { render, screen, fireEvent } from '@testing-library/react'; import '@testing-library/jest-dom'; -import type { ViewFilterOperator } from '@objectstack/spec/ui'; +import type { ViewFilterOperator, ViewFilterRule } from '@objectstack/spec/ui'; import type { FilterBuilderCondition as AuthorityCondition, FilterGroup as AuthorityGroup, @@ -97,12 +97,23 @@ const offVocabulary: FilterBuilderRow = { value: 1, }; -// The value extension: the builder's own scalar-or-list union, REQUIRED — -// ruled over the authority's `value?: any`, which would switch the typing of -// every exported value helper off in silence. -expectType>(); -// @ts-expect-error `value` is required on the builder's row -const rowWithoutValue: FilterBuilderRow = { id: 'c1', field: 'a', operator: 'equals' }; +// The value extension: the SPEC rule's value, by reference (#26306), plus the +// builder's own boolean-bearing list — never the +// authority's `value?: any`, which would switch the typing of every exported +// value helper off in silence. Optional, as the spec's is: a value-less +// operator is authored with no value, and a required member refused the +// authored group the renderer hands over. The per-case pins live in +// `filter-builder-spec-row-26306.test.tsx`. +expectType>(); +const rowWithoutValue: FilterBuilderRow = { id: 'c1', field: 'a', operator: 'is_null' }; +// …and still not `any`: a value the spec refuses is refused here. +const objectValue: FilterBuilderRow = { + id: 'c1', + field: 'a', + operator: 'equals', + // @ts-expect-error `value` is the spec rule's value, not an object + value: { $gt: 1 }, +}; /* ── The group: only `conditions` is restated ─────────────────────────────── */ @@ -123,7 +134,8 @@ describe('objectui#9306 — the builder derives its row and group from @object-u it('the type-level pins above compile (see `tsc -p tsconfig.test.json`); the fixtures are live values', () => { expect(rowWithoutId.field).toBe('a'); expect(offVocabulary.id).toBe('c1'); - expect(rowWithoutValue.operator).toBe('equals'); + expect(rowWithoutValue.operator).toBe('is_null'); + expect(objectValue.id).toBe('c1'); expect(groupWithoutId.conditions).toEqual([]); expect(nested.logic).toBe('and'); }); diff --git a/ui/packages/components/src/__tests__/filter-builder-spec-row-26306.test.tsx b/ui/packages/components/src/__tests__/filter-builder-spec-row-26306.test.tsx new file mode 100644 index 00000000000..e6267077c4c --- /dev/null +++ b/ui/packages/components/src/__tests__/filter-builder-spec-row-26306.test.tsx @@ -0,0 +1,132 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The builder accepts the spec-derived group the `filter-builder` renderer + * hands it (#26306). + * + * The renderer passes `schema.value` — the authored `FilterGroup` from + * `@object-ui/types`, whose row takes its operator from the spec's + * `ViewFilterOperator` — to a builder typed on its own `FilterBuilderRowGroup`. + * The two were assignable in neither direction, and the renderer props' `any` + * index signature hid that. Measured against the spec's `ViewFilterRule` + * (`@objectstack/spec/ui`), the row diverged in three VALUE cases and in no + * operator case a consumer can author: + * + * | case | spec rule | builder row (before) | + * |--------------------------------|------------------------|---------------------------------| + * | value omitted (unary operator) | `value?` | `value` required | + * | value `null` | admitted | refused | + * | boolean entry in a list / pair | refused | admitted (kept, see below) | + * | operator | `VIEW_FILTER_OPERATORS` | those + opt-in `exists` / `notExists` | + * + * The operator row is not a divergence on the way IN: every spec id is a + * builder id. The opt-in pair is the builder's emit-side extension (objectui#9559 + * ruling B) and is pinned in `filter-builder-name-authority-9306.test.tsx`. + * + * The builder's row now takes the spec's value by reference + * (`ViewFilterRule["value"]`), so the two cases that kept an authored group out + * of the builder have one pin each below. Neither was only a type gap: a field + * switch converted the ABSENT value, so a value-less row on a number column + * threw on `.trim()` and on a text column came back as the string + * `"undefined"`. + * + * The third case runs the other way — the builder is WIDER than the spec, so it + * does not keep an authored group out — and is left as it is here: the row + * keeps its boolean list member beside the spec's value, and narrowing it is a + * breaking change to a published type with a consumer in `@object-ui/plugin-list` + * (`UserFilters` hands `LookupValuePicker` a boolean-bearing list). The pin + * below states the width as it is, so the narrowing has to change it. + * + * Type-level pins are judged by `tsc -p tsconfig.test.json`; a green vitest run + * says nothing about them. + */ +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import type { ViewFilterRule } from '@objectstack/spec/ui'; +import type { FilterField, FilterGroup } from '@object-ui/types'; +import { + FilterBuilder, + retypeFilterValue, + type FilterBuilderProps, + type FilterBuilderRow, +} from '../custom/filter-builder'; + +type Equal = + (() => T extends A ? 1 : 2) extends (() => T extends B ? 1 : 2) ? true : false; +function expectType(_: T = true as T): void { /* compile-time only */ } +type Assignable = [From] extends [To] ? true : false; + +/* ── The shapes ───────────────────────────────────────────────────────────── */ + +// The row's value IS the spec rule's value — by reference, not a copy of it — +// plus the builder's own wider list member. +expectType>(); +expectType>(); +// The renderer's two reads cross with no cast: the authored group and the +// authored field list are what the builder's props take. +expectType>>(); +expectType>>(); + +const FIELDS: NonNullable = [ + { value: 'title', label: 'Title', type: 'text' }, + { value: 'amount', label: 'Amount', type: 'number' }, + { value: 'quota', label: 'Quota', type: 'currency' }, +]; + +/** Render ONE authored row, typed as the authored group — no cast. */ +function renderAuthored(row: FilterGroup['conditions'][number]) { + const onChange = vi.fn(); + // Hoisted out of the JSX: the sync effect re-seeds state when the `value` + // prop's identity changes, which would undo the interaction under test. + const value: FilterGroup = { logic: 'and', conditions: [row] }; + render(); + return onChange; +} + +function lastRow(onChange: ReturnType): FilterBuilderRow { + expect(onChange.mock.calls.length, 'the builder never called onChange').toBeGreaterThan(0); + return onChange.mock.calls.at(-1)?.[0].conditions[0]; +} + +/** Drive the row's REAL dropdown at `index` (0 = field). */ +async function pick(index: number, label: string) { + fireEvent.keyDown(screen.getAllByRole('combobox')[index], { key: 'ArrowDown' }); + const option = await waitFor(() => { + const found = screen.getAllByRole('option').find((o) => o.textContent === label); + expect(found, `no "${label}" option in dropdown ${index}`).toBeTruthy(); + return found!; + }); + fireEvent.click(option); +} + +describe('#26306 — the builder accepts the spec-derived row', () => { + it('value omitted: a value-less row keeps no value through a field switch (it used to throw on a number column)', async () => { + const onChange = renderAuthored({ id: 'c1', field: 'title', operator: 'is_null' }); + await pick(0, 'Amount'); + const row = lastRow(onChange); + expect(row.field).toBe('amount'); + expect(row.operator).toBe('is_null'); + expect(row.value).toBeUndefined(); + // The text family, where the absence used to come back spelled out. + expect(retypeFilterValue(undefined, 'text', 'is_null')).toBeUndefined(); + }); + + it('value null: a null row keeps null through a field switch (it used to throw on `.trim()`)', async () => { + const onChange = renderAuthored({ id: 'c1', field: 'amount', operator: 'equals', value: null }); + // Drawn as unfilled, not as the string "null". + expect((document.querySelector('input[placeholder="Value"]') as HTMLInputElement).value).toBe(''); + await pick(0, 'Quota'); + const row = lastRow(onChange); + expect(row.field).toBe('quota'); + expect(row.value).toBeNull(); + }); + +}); diff --git a/ui/packages/components/src/custom/filter-builder.tsx b/ui/packages/components/src/custom/filter-builder.tsx index 3d6f94a5397..208b54d36dd 100644 --- a/ui/packages/components/src/custom/filter-builder.tsx +++ b/ui/packages/components/src/custom/filter-builder.tsx @@ -14,6 +14,7 @@ import { VIEW_FILTER_LIST_VALUE_OPERATORS, VIEW_FILTER_PAIR_VALUE_OPERATORS, normalizeFilterOperator, + type ViewFilterRule, } from "@objectstack/spec/ui" import { expandEmptyOperator } from "@objectstack/spec/data" import { SchemaRendererContext } from "@object-ui/react" @@ -64,11 +65,19 @@ import { Popover, PopoverContent, PopoverTrigger } from "../ui/popover" * protocol has no member for (objectui#9559 ruling B keeps them opt-in and * unfolded). It was `string`, which let a caller hand the builder an id no * dropdown entry can hold. - * - `value` — REQUIRED and narrowed to the scalar-or-list union this builder - * edits, where the authority declares `value?: any`. Every exported helper - * below that takes a row's value (`reshapeFilterValue`, `retypeFilterValue`, + * - `value` — the SPEC's rule value, taken BY REFERENCE + * (`ViewFilterRule["value"]`, `@objectstack/spec/ui`), where the authority + * declares `value?: any`. Every exported helper below that takes a row's + * value (`reshapeFilterValue`, `retypeFilterValue`, * `narrowFilterValueToOptions`, …) is typed on this member, so `any` here - * would switch their checking off in silence. + * would switch their checking off in silence. OPTIONAL and admitting + * `null`, as the spec's is: a value-less operator (`is_null`, `is_empty`, + * …) is authored with no `value`, and a required member refused the + * authored `FilterGroup` the `filter-builder` renderer hands over. One + * member is the builder's own and WIDER than the spec's: a list entry may + * be a boolean, because a boolean scalar carried into a list operator + * (`reshapeFilterValue`, `normalizeToArray`) stays a boolean. The spec's + * list holds strings and numbers only. * * Its own NAME, not the authority's (objectui#6349, batch 12). Under the * authority's name this was a second declaration of `FilterBuilderCondition` @@ -81,7 +90,7 @@ import { Popover, PopoverContent, PopoverTrigger } from "../ui/popover" export interface FilterBuilderRow extends Omit { operator: FilterBuilderOperator - value: string | number | boolean | (string | number | boolean)[] + value?: ViewFilterRule["value"] | (string | number | boolean)[] } /** @@ -874,6 +883,11 @@ export function retypeFilterValue( } default: { const scalar = Array.isArray(value) ? (value[0] ?? "") : value + // A row with no value — the spec's value-less operators are authored + // without one — keeps having none. Converting the absence instead read + // it as a value: the text family spelled `undefined` as the string + // `"undefined"`, and the number family threw on `.trim()`. + if (isValueUnset(scalar)) return scalar return convertScalarToFamily(scalar, family) ?? "" } } @@ -1340,7 +1354,7 @@ function offeredOperatorsForRow( */ function isValueUnset( value: FilterBuilderRow["value"] | undefined | null, -): boolean { +): value is undefined | null | "" { return value === undefined || value === null || value === "" } @@ -1361,7 +1375,7 @@ function displayScalarValue(value: FilterBuilderRow["value"]): string { /** Normalize a filter value into an array for multi-select scenarios */ function normalizeToArray(value: FilterBuilderRow["value"]): (string | number | boolean)[] { if (Array.isArray(value)) return value - if (!isValueUnset(value)) return [value as string | number | boolean] + if (!isValueUnset(value)) return [value] return [] } diff --git a/ui/packages/components/src/renderers/complex/filter-builder.tsx b/ui/packages/components/src/renderers/complex/filter-builder.tsx index 56a41793f00..1700d74514d 100644 --- a/ui/packages/components/src/renderers/complex/filter-builder.tsx +++ b/ui/packages/components/src/renderers/complex/filter-builder.tsx @@ -7,7 +7,7 @@ */ import { ComponentRegistry } from '@object-ui/core'; -import type { FilterBuilderSchema } from '@object-ui/types'; +import type { FilterBuilderSchema, FilterGroup } from '@object-ui/types'; import { useDisplayLocale } from '@object-ui/i18n'; // Aliased on import, following PR #4169's convention (and `AppSchemaRenderer`'s // use of it): this repo has its OWN `resolveKeyedI18nLabel` over a DIFFERENT @@ -17,7 +17,10 @@ import { resolveI18nLabel as resolveInlineI18nLabel } from '@objectstack/spec/ui import { FilterBuilder } from '../../custom/filter-builder'; ComponentRegistry.register('filter-builder', - ({ schema, className, onChange, ...props }: { schema: FilterBuilderSchema; className?: string; onChange?: (event: any) => void; [key: string]: any }) => { + // `value` is declared because it is read by name: a host that binds the node + // (a form field) forwards the group it holds, the same authored `FilterGroup` + // the node's own `value` is. + ({ schema, className, onChange, ...props }: { schema: FilterBuilderSchema; className?: string; onChange?: (event: any) => void; value?: FilterGroup; [key: string]: any }) => { // Read-time resolution against the display locale (objectui#4580 revised // Q1-A). `BaseSchema.label` accepts `string | I18nLabel`; rendering the map // straight into a text node THREW "Objects are not valid as a React child". @@ -41,7 +44,7 @@ ComponentRegistry.register('filter-builder', )}