From e172afaa1cc996b061c769ad06882d41c9b1354a Mon Sep 17 00:00:00 2001 From: Jake Boone Date: Fri, 25 Sep 2026 17:00:52 -0700 Subject: [PATCH 1/4] Address Grok review #4 --- CHANGELOG.md | 7 +++ README.md | 13 ++++ docs/differences-from-react-querybuilder.md | 4 ++ docs/reactivity.md | 63 +++++++++++++++++++ .../src/lib/components/MatchModeEditor.svelte | 2 +- .../src/lib/components/RuleSubQuery.svelte | 9 +-- .../src/lib/components/ValueEditor.svelte | 4 +- .../src/lib/effectBudget.test.ts | 25 ++++++++ .../src/lib/internal/Control.svelte | 8 +-- .../src/lib/reactive/context.svelte.ts | 2 +- .../src/lib/reactive/context.test.ts | 17 ++++- .../createQueryBuilderState.svelte.ts | 23 +++---- .../src/lib/reactive/ruleGroupParts.svelte.ts | 13 ++-- .../src/lib/reactive/ruleParts.svelte.ts | 36 ++++++----- .../lib/reactive/valueEditorEffect.svelte.ts | 1 + .../src/lib/types/props.ts | 39 +++++------- .../src/lib/types/schema.ts | 17 +++-- .../src/lib/types/types.test-d.ts | 2 +- .../test/conformance/cases.ts | 26 ++++++++ .../conformance/classnames-post-flush.test.ts | 5 +- .../test/conformance/classnames.test.ts | 13 +++- 21 files changed, 242 insertions(+), 87 deletions(-) create mode 100644 docs/reactivity.md create mode 100644 packages/svelte-querybuilder/src/lib/effectBudget.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 99b8660..b7cbdaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,13 @@ Control elements are now composed the Svelte way. Each of the 24 control names i - `Rule`, `RuleGroup`, and `ValueEditor` are generic over the same `F`/`O`. `ValueEditor` previously hardcoded `ValueEditorProps`, so a replacement value editor was better typed than the built-in one. - `RuleComponents` takes `mode`, `rule: { props, parts }`, and (in `subQuery` mode) `subQuery: { props, parts }`, replacing four flat props. The two `(props, parts)` pairs — the rule's, and the subquery's own query-builder state — are now visibly paired, and whether a rule renders a `matchModeEditor` is decided by the explicit `mode` discriminator rather than by the presence of the subquery state. - `QueryBuilder` publishes context as `setQueryBuilderContext(() => state.context)` rather than an `Object.defineProperty` reflection loop, so the key set is no longer snapshotted at initialization. `getQueryBuilderContext` returns a getter. +- **Breaking:** every `context` is typed `unknown` instead of `any` — the free-form `context` prop on subcomponents and `QueryBuilder`, the `context` argument to `onAddRule`/`onAddGroup`/`onMoveRule`/`onMoveGroup`/`onGroupRule`/`onGroupGroup`/`onRemove`, and `ActionProps.handleOnClick`'s second argument. `onLog` takes `unknown`. Narrow before use. +- **Breaking:** `Schema.getSubQueryBuilderProps` returns the new, exported `SubQueryBuilderProps` type and accepts any field name (`string`), and the internal implementation no longer returns `any`. Removes every `as never` from `RuleSubQuery`. +- `Control` (the internal renderer) is generic over the control's props, so each built-in call site's props are type-checked against the control's prop type instead of passing through `any`. This caught `MatchModeEditor`'s threshold editor passing `valueSource` as `string` rather than `ValueSource`. +- Remaining casts and `any`s in `src/lib` are either removed or annotated with the reason they stay, which is almost always a match for an `any` in core (`RuleType.value`, `QueryActions`). +- A unit test caps `$effect` use in `src/lib` to an explicit allow-list (today, the single value-editor reset). Another checks this package's control keys against core's `controlKeys`/`controlKind` at runtime, alongside the existing compile-time guard. +- When a conformance class-name comparison fails only on whitespace in an element's own text, the failure message now points to the `` joiners. +- New [Reactivity notes](./docs/reactivity.md) doc, and a "Stability and versioning" README section covering the core version policy and what blocks 1.0. - Minimum `@react-querybuilder/core` is now 8.23.0, for the query-tool `freeze` opt-out (deep-freezing a Svelte `$state` proxy throws), `shouldCoalesce`, `controlKeys`/`controlKind`, and `DefaultFieldProp`/`DefaultOperatorProp`. ### Fixed diff --git a/README.md b/README.md index 915d340..47e0d66 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,7 @@ The DOM is class-compatible with React Query Builder, so existing RQB stylesheet - [Differences from React Query Builder](./docs/differences-from-react-querybuilder.md) — start here if you know RQB - [Customization](./docs/customization.md) — snippets, `controls`, translations, context - [Styling](./docs/styling.md) +- [Reactivity notes](./docs/reactivity.md) — stable `query` references, context getters, writing custom controls - Concepts, field/operator configuration, query formats, and parsers: the [React Query Builder documentation](https://react-querybuilder.js.org/docs/intro) applies directly, since the logic layer is shared. ## Examples @@ -85,6 +86,18 @@ Not in v1, and not planned for the near term: - A Redux store or a `qbId` registry — hold the query yourself and use `bind:query` - Deprecated props carried over from React Query Builder +## Stability and versioning + +Pre-1.0: minor releases (`0.x`) may break the component API. Breaking changes are listed in [`CHANGELOG.md`](./CHANGELOG.md). + +**Core version policy.** `@react-querybuilder/core` is a caret dependency on a minor (currently `^8.23.0`). The conformance fixtures are pinned to the matching React Query Builder tag, and the two move together: raising the core minimum means bumping the fixture tag and passing conformance against it. Core's public API, including the headless derivations this package uses, is semver-covered upstream. + +**What blocks 1.0:** + +- A release with no breaking change to the component API: props, control names and their prop types, `Schema`, the `create*Parts` helpers, and the context functions. +- DOM conformance green against the then-current React Query Builder release, with the text channel enabled. +- The non-goals above staying non-goals. 1.0 doesn't depend on any of them, and adding one later won't be a breaking change. + ## License MIT diff --git a/docs/differences-from-react-querybuilder.md b/docs/differences-from-react-querybuilder.md index 562f46f..f9df5f1 100644 --- a/docs/differences-from-react-querybuilder.md +++ b/docs/differences-from-react-querybuilder.md @@ -114,12 +114,16 @@ See [customization.md](./customization.md) for the full resolution order. - `ActionProps.handleOnClick` and `ShiftActionsProps.shiftUp`/`shiftDown` take a DOM `MouseEvent`, not React's synthetic `MouseEvent`. - `Controls` entries are uniformly nullable, `null` meaning "render nothing". Unlike React, `undoRedoActions` has a default implementation, so it is never unset. - `ControlElementsProp` → `ControlsProp` (the `controls` prop), plus `ControlSnippetProps`, which has no React counterpart: one top-level `Snippet<[props]>` prop per control name. +- Every `context` (the free-form prop, and the argument to the `on*` callbacks and `handleOnClick`) is `unknown` rather than `any`. Narrow before use. +- `Schema.getSubQueryBuilderProps` returns the named `SubQueryBuilderProps` type and accepts any field name (`string`). - A resolved control is `Control

= Component

| { snippet: Snippet<[P]> }`, or `null` for "render nothing"; `ControlPropsMap` is the single source of truth for control names and their props. ## Reactivity React's hooks have no direct equivalents, and the `useMemo` graphs in `Rule`/`RuleGroup` are not ported — Svelte's reactivity is fine-grained, so manual memoization is unnecessary. If you were reaching into `useRule`/`useRuleGroup` to build a custom component, the equivalents are `createRuleParts` and `createRuleGroupParts`, which take a props _getter_ rather than a props object. +See [Reactivity notes](./reactivity.md) for the consumer-facing consequences: stable `query` references, context as a getter, and getter-backed control props. + ## Known behavioral note Structural options — `fields`, `operators`, `combinators`, `translations`, `maxLevels`, `disabled`, and the boolean flags — are derived from props, so changing one mid-session updates both the rendered selectors and the defaults assigned to newly created rules without touching the query or the undo/redo history. A config-only change does not fire `onQueryChange`. diff --git a/docs/reactivity.md b/docs/reactivity.md new file mode 100644 index 0000000..819f483 --- /dev/null +++ b/docs/reactivity.md @@ -0,0 +1,63 @@ +# Reactivity notes + +Most apps never need this page. It covers the cases where Svelte's reactivity model shows through the component API: driving the query from outside, publishing context, and writing custom controls or replacement `rule`/`ruleGroup` components. + +## Keep the `query` reference stable + +The `query` prop is an _input_, not the authority: whenever it **changes**, it wins over local edits. "Changes" means a new reference that is also structurally different from what the builder last rendered. + +So don't build a new object each time the prop is read: + +```svelte + + (myQuery = q)} /> + + + (myQuery = q)} /> + +``` + +Reassign the variable only when the query actually changes. + +## `$state` vs. `$state.raw` for the query + +Both work. Queries are immutable and replaced wholesale, so `$state.raw` is the better fit: no deep proxy, and reference equality means what it says. With deep `$state`, the builder hands the query back to you and gets a proxy of it in return. It recognizes that proxy by structure, so nothing breaks; it just costs a comparison. + +If you edit the query yourself with core's tools (`add`, `update`, …) and it holds `$state` proxies, pass `{ freeze: false }`. Immer's deep freeze throws on Svelte proxies. + +## Context is a getter + +`setQueryBuilderContext` takes a function, not a value, and `getQueryBuilderContext()` returns that function. Svelte context is set once, during component initialization, but the configuration it carries changes over time. Calling the getter _inside_ a `$derived` or the template is what makes the read reactive: + +```ts +const getContext = getQueryBuilderContext(); +// Right: re-read, and tracked, on every evaluation. +const showNotToggle = $derived(getContext?.()?.showNotToggle ?? false); +// Wrong: captures the value at init and never updates. +const snapshot = getContext?.()?.showNotToggle; +``` + +Nested builders, including the subquery builders that match modes create, rely on this to inherit changes from the outer builder. See [customization.md](./customization.md#applying-customization-to-a-subtree). + +## Control props are getter-backed + +Built-in components pass props to each control as an object whose properties are getters. A control only depends on the props it actually reads, so a change to one rule's value doesn't re-run every other control's derivations. On large queries this makes mounting much faster (about 40x in the reset-heavy benchmark). + +For custom controls, this means: + +- **Read props where you use them**: in the template, in `$derived`, or in an event handler. A leaf control can destructure `$props()`, since it reads everything during render anyway. +- **Don't copy a prop into a plain variable at init** (`const v = props.value`). It won't update, and Svelte warns about it (`state_referenced_locally`). +- **When forwarding props to another component, don't destructure.** Keep `const props = $props()` and pass `props.x` where it's needed. Destructuring reads every prop immediately, which subscribes the component to all of them. +- **`{...props}` is fine when you need everything.** It reads every getter, so the target depends on everything, which is correct if it uses everything. + +## Replacement `rule` / `ruleGroup` components + +`createRuleParts` and `createRuleGroupParts` take a props _getter_ (`() => props`), for the same reason context does: the parts are `$derived` from props and must read them late. Pass `() => props`, not `props`. + +## `context` is `unknown` + +The free-form `context` prop, and the `context` argument to the `on*` callbacks and `handleOnClick`, are typed `unknown`. Narrow before use: + +```ts +const ctx = props.context as { locale?: string } | undefined; +``` diff --git a/packages/svelte-querybuilder/src/lib/components/MatchModeEditor.svelte b/packages/svelte-querybuilder/src/lib/components/MatchModeEditor.svelte index 31c6449..80e7fbc 100644 --- a/packages/svelte-querybuilder/src/lib/components/MatchModeEditor.svelte +++ b/packages/svelte-querybuilder/src/lib/components/MatchModeEditor.svelte @@ -82,7 +82,7 @@ field: '', operator: '', value: thresholdNum, - valueSource: 'value', + valueSource: 'value' as const, fieldData: thresholdFieldData, schema: thresholdSchema, path: dummyPath, diff --git a/packages/svelte-querybuilder/src/lib/components/RuleSubQuery.svelte b/packages/svelte-querybuilder/src/lib/components/RuleSubQuery.svelte index d194a77..ef5df48 100644 --- a/packages/svelte-querybuilder/src/lib/components/RuleSubQuery.svelte +++ b/packages/svelte-querybuilder/src/lib/components/RuleSubQuery.svelte @@ -26,17 +26,14 @@ const schema = $derived(props.schema); const subQueryBuilderProps = $derived( - schema.getSubQueryBuilderProps(props.rule.field as never, { - fieldData: parts.fieldData as never, - }) as Record + schema.getSubQueryBuilderProps(props.rule.field, { fieldData: parts.fieldData }) ); const subproperties = $derived( prepareOptionList({ placeholder: props.translations.fields, - optionList: (parts.fieldData.subproperties ?? - subQueryBuilderProps.fields ?? - defaultSubproperties) as never, + optionList: + parts.fieldData.subproperties ?? subQueryBuilderProps.fields ?? defaultSubproperties, autoSelectOption: schema.autoSelectField || !!parts.fieldData.subproperties, }).optionList ); diff --git a/packages/svelte-querybuilder/src/lib/components/ValueEditor.svelte b/packages/svelte-querybuilder/src/lib/components/ValueEditor.svelte index 4f83b16..00df501 100644 --- a/packages/svelte-querybuilder/src/lib/components/ValueEditor.svelte +++ b/packages/svelte-querybuilder/src/lib/components/ValueEditor.svelte @@ -20,7 +20,7 @@ import Control from '../internal/Control.svelte'; import Label from '../internal/Label.svelte'; import { createValueEditorReset } from '../reactive/valueEditorEffect.svelte.js'; - import type { ValueEditorProps, ValueSelectorProps } from '../types/props.js'; + import type { ValueEditorProps } from '../types/props.js'; const props: ValueEditorProps = $props(); @@ -86,7 +86,7 @@ field: props.field, fieldData: props.fieldData, rule: props.rule, - } as unknown as ValueSelectorProps); + }); const isBetween = $derived( (props.operator === 'between' || props.operator === 'notBetween') && diff --git a/packages/svelte-querybuilder/src/lib/effectBudget.test.ts b/packages/svelte-querybuilder/src/lib/effectBudget.test.ts new file mode 100644 index 0000000..49fe8f7 --- /dev/null +++ b/packages/svelte-querybuilder/src/lib/effectBudget.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, it } from 'vitest'; + +// Every published source file, as text. Tests and test harnesses excluded. +const sources = import.meta.glob( + ['./**/*.svelte', './**/*.ts', '!./**/*.test.ts', '!./**/*.test.svelte', '!./**/*.test-d.ts'], + { query: '?raw', import: 'default', eager: true } +); + +/** + * Where `$effect` is allowed, and how many times. Prefer `$derived`; raising a count here needs + * a reason in review. `valueEditorEffect` resets a value after an operator/type change, which is + * genuinely a side effect. + */ +const effectBudget: Record = { './reactive/valueEditorEffect.svelte.ts': 1 }; + +describe('$effect budget', () => { + it('matches the allow-list exactly', () => { + const actual: Record = {}; + for (const [path, text] of Object.entries(sources)) { + const n = text.match(/\$effect(\.pre)?\s*\(/g)?.length ?? 0; + if (n > 0) actual[path] = n; + } + expect(actual).toEqual(effectBudget); + }); +}); diff --git a/packages/svelte-querybuilder/src/lib/internal/Control.svelte b/packages/svelte-querybuilder/src/lib/internal/Control.svelte index 2e94da3..be2061b 100644 --- a/packages/svelte-querybuilder/src/lib/internal/Control.svelte +++ b/packages/svelte-querybuilder/src/lib/internal/Control.svelte @@ -8,14 +8,10 @@ `props` is forwarded as a single object, matching the snippet's single-argument signature. --> - {#if control == null} diff --git a/packages/svelte-querybuilder/src/lib/reactive/context.svelte.ts b/packages/svelte-querybuilder/src/lib/reactive/context.svelte.ts index 0e691a5..2272592 100644 --- a/packages/svelte-querybuilder/src/lib/reactive/context.svelte.ts +++ b/packages/svelte-querybuilder/src/lib/reactive/context.svelte.ts @@ -181,7 +181,7 @@ export const mergeTranslations = ( contextT?: Partial ): TranslationsFull => mergeAnyTranslations( - defaultTranslations as unknown as Record>, + defaultTranslations as Record>, contextT as Record> | undefined, propsT as Record> | undefined ) as unknown as TranslationsFull; diff --git a/packages/svelte-querybuilder/src/lib/reactive/context.test.ts b/packages/svelte-querybuilder/src/lib/reactive/context.test.ts index 3e95db9..a04851e 100644 --- a/packages/svelte-querybuilder/src/lib/reactive/context.test.ts +++ b/packages/svelte-querybuilder/src/lib/reactive/context.test.ts @@ -1,6 +1,12 @@ -import { defaultTranslations, standardClassnames } from '@react-querybuilder/core'; +import { + controlKeys, + controlKind, + defaultTranslations, + standardClassnames, +} from '@react-querybuilder/core'; import type { Snippet } from 'svelte'; import { describe, expect, it } from 'vitest'; +import { defaultControlElements } from '../components/defaultControlElements.js'; import { getQueryBuilderContext, mergeControls, @@ -139,6 +145,15 @@ describe('mergeControls', () => { // Controls that only exist upstream are never included. expect('dragHandle' in controls).toBe(false); }); + + // Runtime twin of the compile-time `AllControlKeysAccountedFor` guard. + it("accounts for every one of core's control keys", () => { + const implemented = Object.keys(mergeControls()); + const excluded = ['dragHandle', 'ruleGroupBodyElements', 'ruleGroupHeaderElements']; + expect([...implemented, ...excluded].sort()).toEqual([...controlKeys].sort()); + expect(Object.keys(defaultControlElements).sort()).toEqual([...implemented].sort()); + for (const key of implemented) expect(key in controlKind).toBe(true); + }); }); describe('mergeTranslations', () => { diff --git a/packages/svelte-querybuilder/src/lib/reactive/createQueryBuilderState.svelte.ts b/packages/svelte-querybuilder/src/lib/reactive/createQueryBuilderState.svelte.ts index 5caedf6..b327f50 100644 --- a/packages/svelte-querybuilder/src/lib/reactive/createQueryBuilderState.svelte.ts +++ b/packages/svelte-querybuilder/src/lib/reactive/createQueryBuilderState.svelte.ts @@ -52,7 +52,7 @@ import { } from '@react-querybuilder/core'; import type { Controls } from '../types/controls.js'; import type { QueryBuilderContextProps, QueryBuilderProps } from '../types/props.js'; -import type { QueryHistory, Schema } from '../types/schema.js'; +import type { QueryHistory, Schema, SubQueryBuilderProps } from '../types/schema.js'; import type { LabelNode, TranslationsFull } from '../types/translations.js'; import type { MergedQueryBuilderConfig } from './context.svelte.js'; import { getQueryBuilderContext, mergeQueryBuilderConfig } from './context.svelte.js'; @@ -166,7 +166,7 @@ export const createQueryBuilderState = < // direction only, so the widening happens once, here, rather than at each of the ~20 call // sites. Read through `getProps()` so a changed callback prop takes effect immediately. const callbacks = $derived( - getProps() as unknown as { + getProps() as { getDefaultField?: DefaultFieldProp; getDefaultOperator?: DefaultOperatorProp; getDefaultValue?: (rule: RuleType, misc: { fieldData: F }) => unknown; @@ -193,7 +193,7 @@ export const createQueryBuilderState = < misc: { fieldData: F } ) => FlexibleOptionList