From b38cb2c1a8fd58fd3f5399d0cda9619c52218126 Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Thu, 16 Jan 2025 10:52:18 -0800 Subject: [PATCH 1/9] engine: `SelectDefinition` (body) -> `SelectControlDefinition` etc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is an engine-internal reorganization to improve some less-than-ideal naming and code structure around parsing selects, and helping to prepare for support of ``. - Addresses a naming conflict between `SelectDefinition` (as a subtype of `NodeDefinition`) and `SelectDefinition` (as a parsed representation of `` and `` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Support for non-string bind/value types was introduced in #273. That support exceeds the current ODK XForms spec, which specifies both controls as supporting only a string type. We initially decided to merge that expansion on these bases: - XLSForms/pyxform do not produce forms with non-string select types, so it would only occur for forms authored as XML XForms, and would presumably be an intentional form design decision - The behavior (tested with `` + ``) is consistent with Collect/JavaRosa We have since decided to revert that for now, because it would violate assumptions about _multiple values_ (from a `` controls. @@ -14,17 +12,15 @@ import type { CodecDecoder, CodecEncoder } from '../ValueCodec.ts'; * to the provided {@link baseCodec}, ensuring that select value types are * treated consistently with the same underlying data types for other controls. */ -export class MultipleValueSelectCodec extends ValueArrayCodec { - constructor(baseCodec: SharedValueCodec) { - const encodeValue: CodecEncoder> = (value) => { - return value.map(baseCodec.encodeValue).join(' '); +export class MultipleValueSelectCodec extends BaseSelectCodec { + constructor(baseCodec: SharedValueCodec<'string'>) { + const encodeValue: CodecEncoder = (value) => { + return value.join(' '); }; - const decodeValue: CodecDecoder> = (value) => { - const instanceValues = xmlXPathWhitespaceSeparatedList(value, { + const decodeValue: CodecDecoder = (value) => { + return xmlXPathWhitespaceSeparatedList(value, { ignoreEmpty: true, }); - - return instanceValues.map(this.decodeItemValue); }; super(baseCodec, encodeValue, decodeValue); diff --git a/packages/xforms-engine/src/lib/codecs/select/SingleValueSelectCodec.ts b/packages/xforms-engine/src/lib/codecs/select/SingleValueSelectCodec.ts index 500015f48..f1e3fe8b2 100644 --- a/packages/xforms-engine/src/lib/codecs/select/SingleValueSelectCodec.ts +++ b/packages/xforms-engine/src/lib/codecs/select/SingleValueSelectCodec.ts @@ -1,21 +1,20 @@ -import type { ValueType } from '../../../client/ValueType.ts'; import type { SharedValueCodec } from '../getSharedValueCodec.ts'; -import { ValueArrayCodec, type RuntimeItemValue, type RuntimeValues } from '../ValueArrayCodec.ts'; import { type CodecDecoder, type CodecEncoder } from '../ValueCodec.ts'; +import { BaseSelectCodec } from './BaseSelectCodec.ts'; import type { MultipleValueSelectCodec } from './MultipleValueSelectCodec.ts'; // prettier-ignore -export type SingleValueSelectRuntimeValues = +export type SingleValueSelectRuntimeValues = | readonly [] - | readonly [RuntimeItemValue]; + | readonly [string]; /** * @see {@link encodeValueFactory} */ // prettier-ignore -type SingleValueSelectCodecValues = - | RuntimeValues - | SingleValueSelectRuntimeValues; +type SingleValueSelectCodecValues = + | SingleValueSelectRuntimeValues + | readonly string[]; /** * @todo This is more permissive than it should be, allowing an array of any @@ -24,9 +23,9 @@ type SingleValueSelectCodecValues = * than one value to be set, this is where we'd start looking. The check is * skipped for now, to reduce performance overhead. */ -const encodeValueFactory = ( - baseCodec: SharedValueCodec -): CodecEncoder> => { +const encodeValueFactory = ( + baseCodec: SharedValueCodec<'string'> +): CodecEncoder => { return (values) => { const [value] = values; @@ -51,21 +50,16 @@ const encodeValueFactory = ( * 2. as an optimization, as the more general implementation performs poorly on * forms which we monitor for performance. */ -export class SingleValueSelectCodec extends ValueArrayCodec< - V, - SingleValueSelectCodecValues -> { - constructor(baseCodec: SharedValueCodec) { +export class SingleValueSelectCodec extends BaseSelectCodec { + constructor(baseCodec: SharedValueCodec<'string'>) { const encodeValue = encodeValueFactory(baseCodec); - const decodeValue: CodecDecoder> = (value) => { - const decoded = baseCodec.decodeValue(value); - - if (decoded == null) { + const decodeValue: CodecDecoder = (value) => { + if (value == null) { return []; } - return [decoded]; + return [value]; }; super(baseCodec, encodeValue, decodeValue); diff --git a/packages/xforms-engine/src/lib/codecs/select/getSelectCodec.ts b/packages/xforms-engine/src/lib/codecs/select/getSelectCodec.ts index 4d6a8922b..35e7c4d29 100644 --- a/packages/xforms-engine/src/lib/codecs/select/getSelectCodec.ts +++ b/packages/xforms-engine/src/lib/codecs/select/getSelectCodec.ts @@ -1,69 +1,27 @@ import { UnreachableError } from '@getodk/common/lib/error/UnreachableError.ts'; import type { SelectDefinition } from '../../../client/SelectNode.ts'; -import type { ValueType } from '../../../client/ValueType.ts'; import { sharedValueCodecs } from '../getSharedValueCodec.ts'; import { MultipleValueSelectCodec } from './MultipleValueSelectCodec.ts'; import { SingleValueSelectCodec } from './SingleValueSelectCodec.ts'; -type MultipleValueSelectCodecs = { - readonly [V in ValueType]: MultipleValueSelectCodec; -}; - -const multipleValueSelectCodecs: MultipleValueSelectCodecs = { - string: new MultipleValueSelectCodec(sharedValueCodecs.string), - int: new MultipleValueSelectCodec(sharedValueCodecs.int), - decimal: new MultipleValueSelectCodec(sharedValueCodecs.decimal), - boolean: new MultipleValueSelectCodec(sharedValueCodecs.boolean), - date: new MultipleValueSelectCodec(sharedValueCodecs.date), - time: new MultipleValueSelectCodec(sharedValueCodecs.time), - dateTime: new MultipleValueSelectCodec(sharedValueCodecs.dateTime), - geopoint: new MultipleValueSelectCodec(sharedValueCodecs.geopoint), - geotrace: new MultipleValueSelectCodec(sharedValueCodecs.geotrace), - geoshape: new MultipleValueSelectCodec(sharedValueCodecs.geoshape), - binary: new MultipleValueSelectCodec(sharedValueCodecs.binary), - barcode: new MultipleValueSelectCodec(sharedValueCodecs.barcode), - intent: new MultipleValueSelectCodec(sharedValueCodecs.intent), -}; +const multipleValueSelectCodec = new MultipleValueSelectCodec(sharedValueCodecs.string); -type SingleValueSelectCodecs = { - readonly [V in ValueType]: SingleValueSelectCodec; -}; - -const singleValueSelectCodecs: SingleValueSelectCodecs = { - string: new SingleValueSelectCodec(sharedValueCodecs.string), - int: new SingleValueSelectCodec(sharedValueCodecs.int), - decimal: new SingleValueSelectCodec(sharedValueCodecs.decimal), - boolean: new SingleValueSelectCodec(sharedValueCodecs.boolean), - date: new SingleValueSelectCodec(sharedValueCodecs.date), - time: new SingleValueSelectCodec(sharedValueCodecs.time), - dateTime: new SingleValueSelectCodec(sharedValueCodecs.dateTime), - geopoint: new SingleValueSelectCodec(sharedValueCodecs.geopoint), - geotrace: new SingleValueSelectCodec(sharedValueCodecs.geotrace), - geoshape: new SingleValueSelectCodec(sharedValueCodecs.geoshape), - binary: new SingleValueSelectCodec(sharedValueCodecs.binary), - barcode: new SingleValueSelectCodec(sharedValueCodecs.barcode), - intent: new SingleValueSelectCodec(sharedValueCodecs.intent), -}; +const singleValueSelectCodec = new SingleValueSelectCodec(sharedValueCodecs.string); // prettier-ignore -export type SelectCodec = - | MultipleValueSelectCodec - | SingleValueSelectCodec; - -export const getSelectCodec = ( - definition: SelectDefinition -): SelectCodec => { - const selectType = definition.bodyElement.type; - const valueType = definition.valueType; +export type SelectCodec = + | MultipleValueSelectCodec + | SingleValueSelectCodec; - switch (selectType) { +export const getSelectCodec = (definition: SelectDefinition<'string'>): SelectCodec => { + switch (definition.bodyElement.type) { case 'select': - return multipleValueSelectCodecs[valueType]; + return multipleValueSelectCodec; case 'select1': - return singleValueSelectCodecs[valueType]; + return singleValueSelectCodec; default: - throw new UnreachableError(selectType); + throw new UnreachableError(definition.bodyElement.type); } }; diff --git a/packages/xforms-engine/src/lib/reactivity/createSelectItems.ts b/packages/xforms-engine/src/lib/reactivity/createSelectItems.ts index 8ec64efe8..61c9a62a8 100644 --- a/packages/xforms-engine/src/lib/reactivity/createSelectItems.ts +++ b/packages/xforms-engine/src/lib/reactivity/createSelectItems.ts @@ -2,9 +2,8 @@ import { UpsertableMap } from '@getodk/common/lib/collections/UpsertableMap.ts'; import type { Accessor } from 'solid-js'; import { createMemo } from 'solid-js'; import type { ActiveLanguage } from '../../client/FormLanguage.ts'; -import type { SelectItem, SelectValueOptions } from '../../client/SelectNode.ts'; +import type { SelectItem } from '../../client/SelectNode.ts'; import type { TextRange as ClientTextRange } from '../../client/TextRange.ts'; -import type { ValueType } from '../../client/ValueType.ts'; import type { EvaluationContext } from '../../instance/internal-api/EvaluationContext.ts'; import type { TranslationContext } from '../../instance/internal-api/TranslationContext.ts'; import type { SelectControl } from '../../instance/SelectControl.ts'; @@ -12,9 +11,8 @@ import { TextChunk } from '../../instance/text/TextChunk.ts'; import { TextRange } from '../../instance/text/TextRange.ts'; import type { EngineXPathNode } from '../../integration/xpath/adapter/kind.ts'; import type { EngineXPathEvaluator } from '../../integration/xpath/EngineXPathEvaluator.ts'; -import type { ItemDefinition } from '../../parse/body/control/select/ItemDefinition.ts'; -import type { ItemsetDefinition } from '../../parse/body/control/select/ItemsetDefinition.ts'; -import type { SelectCodec } from '../codecs/select/getSelectCodec.ts'; +import type { ItemDefinition } from '../../parse/body/control/ItemDefinition.ts'; +import type { ItemsetDefinition } from '../../parse/body/control/ItemsetDefinition.ts'; import { createComputedExpression } from './createComputedExpression.ts'; import type { ReactiveScope } from './scope.ts'; import { createTextRange } from './text/createTextRange.ts'; @@ -45,8 +43,8 @@ interface SourceValueSelectItem { readonly label: ClientTextRange<'item-label'>; } -const createTranslatedStaticSelectItems = ( - select: SelectControl, +const createTranslatedStaticSelectItems = ( + select: SelectControl, items: readonly ItemDefinition[] ): Accessor => { return select.scope.runTask(() => { @@ -66,7 +64,7 @@ const createTranslatedStaticSelectItems = ( }); }; -class ItemsetItemEvaluationContext implements EvaluationContext { +class ItemsetItemEvaluationContext implements EvaluationContext { readonly isAttached: Accessor; readonly scope: ReactiveScope; readonly evaluator: EngineXPathEvaluator; @@ -74,7 +72,7 @@ class ItemsetItemEvaluationContext implements EvaluationCon readonly getActiveLanguage: Accessor; constructor( - select: SelectControl, + select: SelectControl, readonly contextNode: EngineXPathNode ) { this.isAttached = select.isAttached; @@ -106,8 +104,8 @@ interface ItemsetItem { value(): string; } -const createItemsetItems = ( - select: SelectControl, +const createItemsetItems = ( + select: SelectControl, itemset: ItemsetDefinition ): Accessor => { return select.scope.runTask(() => { @@ -135,8 +133,8 @@ const createItemsetItems = ( }); }; -const createItemset = ( - select: SelectControl, +const createItemset = ( + select: SelectControl, itemset: ItemsetDefinition ): Accessor => { return select.scope.runTask(() => { @@ -165,31 +163,12 @@ const createItemset = ( * their appropriate dependencies (whether relative to the itemset item node, * referencing a form's `itext` translations, etc). */ -export const createSelectItems = ( - select: SelectControl, - codec: SelectCodec -): Accessor> => { +export const createSelectItems = (select: SelectControl): Accessor => { const { items, itemset } = select.definition.bodyElement; - let getSourceValueItems: Accessor; - if (itemset != null) { - getSourceValueItems = createItemset(select, itemset); - } else { - getSourceValueItems = createTranslatedStaticSelectItems(select, items); + return createItemset(select, itemset); } - return select.scope.runTask(() => { - return createMemo(() => { - return getSourceValueItems().map((item): SelectItem => { - const asString = item.value; - - return { - value: codec.decodeItemValue(asString), - label: item.label, - asString, - }; - }); - }); - }); + return createTranslatedStaticSelectItems(select, items); }; diff --git a/packages/xforms-engine/test/lib/reactivity/createSelectItems.test.ts b/packages/xforms-engine/test/lib/reactivity/createSelectItems.test.ts index 6ef1c7d82..c53c925bb 100644 --- a/packages/xforms-engine/test/lib/reactivity/createSelectItems.test.ts +++ b/packages/xforms-engine/test/lib/reactivity/createSelectItems.test.ts @@ -249,11 +249,10 @@ describe('createSelectItems - reactive ` values are written in option order (consistent with JavaRosa, also tested elsewhere with strings)', - }, - { selectValues: [], expectedValue: [] }, - ])('setValue ($selectValues)', ({ selectValues, expectedValue, reason }) => { - const selectValuesDescription = JSON.stringify(selectValues.map(String)); - const expectedValueDescription = JSON.stringify(expectedValue.map(String)); - const reasonDescription = reason == null ? '' : `(${reason})`; - const description = - `sets ${selectValuesDescription}, resulting in value ${expectedValueDescription} ${reasonDescription}`.trim(); - - it(description, () => { - scenario.proposed_answerTypedSelect('/root/int-value', 'int', selectValues); - answer = getTypedSelectNodeAnswer('/root/int-value', 'int'); - - expectTypeOf(answer.value).toEqualTypeOf(); - - expect(answer.value).toEqual(expectedValue); - - const expectedStringValue = expectedValue.map(String).join(' '); - - expect(answer.stringValue).toBe(expectedStringValue); - }); - }); - - interface SetIntSelectErrorCase { - readonly selectValues: readonly bigint[]; - } - - // TODO: unsure if there's anything to test here! The values won't be - // set because they're not available in the select's `valueOptions`. - describe.skip.each([ - { selectValues: [-2_147_483_649n] }, - { selectValues: [2_147_483_648n] }, - { selectValues: [-2_147_483_649n, 2_147_483_648n] }, - { selectValues: [2_147_483_649n, -2_147_483_648n] }, - ])('integer value out of specified bounds ($selectType)', ({ selectValues }) => { - const selectValuesDescription = JSON.stringify(selectValues.map(String)); - - it(`fails to set ${selectValuesDescription}`, () => { - let caught: unknown; - - try { - scenario.proposed_answerTypedSelect('/root/int-value', 'int', selectValues); - answer = getTypedSelectNodeAnswer('/root/int-value', 'int'); - } catch (error) { - caught = error; - } - - expect(caught, `Value was set to ${answer.stringValue}`).toBeInstanceOf(Error); - }); - }); - }); - }); - - describe('type="decimal"', () => { - let answer: SelectNodeAnswer<'decimal'>; - - beforeEach(() => { - answer = getTypedSelectNodeAnswer('/root/decimal-value', 'decimal'); - }); - - it('has a runtime value which is an array of a single number value', () => { - expect(answer.value).toBeInstanceOf(Array); - expect(answer.value.length).toBe(1); - expect(answer.value[0]).toBeTypeOf('number'); - }); - - it('has a readonly number[] static type', () => { - expectTypeOf(answer.value).toEqualTypeOf(); - }); - - it('has a number populated value', () => { - expect(answer.value).toEqual([45.67]); - }); - - it('has an empty array blank value', () => { - scenario.answer(selectRelevancePath, 'no'); - answer = getTypedSelectNodeAnswer('/root/decimal-value', 'decimal'); - expect(answer.value).toEqual([]); - }); - - describe('setting decimal values', () => { - interface SetDecimalSelectValueCase { - readonly selectValues: readonly number[]; - readonly expectedValue: readonly number[]; - readonly reason?: string; - } - - it.each([ - { selectValues: [89], expectedValue: [89] }, - { selectValues: [10], expectedValue: [10] }, - { selectValues: [45.67, 23.4], expectedValue: [45.67, 23.4] }, - { - selectValues: [23.4, 45.67], - expectedValue: [45.67, 23.4], - reason: - '