Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert select value type support #277

Merged
merged 9 commits into from
Jan 20, 2025
Merged

Revert select value type support #277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b38cb2c
engine: `SelectDefinition` (body) -> `SelectControlDefinition` etc
eyelidlessness Jan 16, 2025
05abe44
engine: roll back support for non-string `<select>` and `<select1>`
eyelidlessness Jan 16, 2025
aa0dbec
scenario: integrate rolled back support for non-string selects
eyelidlessness Jan 16, 2025
3abb3b0
web-forms (Vue UI): integrate rolled back support for non-string selects
eyelidlessness Jan 16, 2025
b4a0d2b
Update select types fixture: we now expect it to produce an error on …
eyelidlessness Jan 16, 2025
c2be258
Update changeset from #273
eyelidlessness Jan 16, 2025
7d84ad3
Add demo fixture for select1 with spaces
eyelidlessness Jan 16, 2025
e6d01b0
Another changeset correction; maybe rename will be picked up by GH bot
eyelidlessness Jan 16, 2025
0a6010e
engine (fix): “range” is not an unsupported control node type
eyelidlessness Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/xforms-engine/src/client/SelectNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type { RuntimeValue } from '../lib/codecs/getSharedValueCodec.ts';
import type {
AnySelectDefinition,
AnySelectControlDefinition,
SelectType,
} from '../parse/body/control/select/SelectDefinition.ts';
} from '../parse/body/control/SelectControlDefinition.ts';
import type { LeafNodeDefinition } from '../parse/model/LeafNodeDefinition.ts';
import type { BaseValueNode, BaseValueNodeState } from './BaseValueNode.ts';
import type { NodeAppearances } from './NodeAppearances.ts';
Expand Down Expand Up @@ -56,7 +56,7 @@ export interface SelectNodeState<V extends ValueType> extends BaseValueNodeState
}

export interface SelectDefinition<V extends ValueType = ValueType> extends LeafNodeDefinition<V> {
readonly bodyElement: AnySelectDefinition;
readonly bodyElement: AnySelectControlDefinition;
}

export type SelectNodeAppearances = NodeAppearances<SelectDefinition>;
Expand Down
10 changes: 0 additions & 10 deletions packages/xforms-engine/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,3 @@ export type * from './client/unsupported/RankNode.ts';
export type * from './client/unsupported/UploadNode.ts';
export type * from './client/validation.ts';
export type * from './client/ValueType.ts';

// TODO: notwithstanding potential conflicts with parallel work on `web-forms`
// (former `ui-vue`), these are the last remaining references **outside of
// `xforms-engine`** to anything besides /client/* and the `initializeForm`
// entrypoint implementation. We'll refine the various `definition` types in due
// time.
export type {
AnySelectDefinition,
SelectDefinition,
} from './parse/body/control/select/SelectDefinition.ts';
2 changes: 1 addition & 1 deletion packages/xforms-engine/src/instance/SelectControl.ts
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

I'm not suggesting this as a significant change, but rather as an improvement to help new contributors and streamline the process. Grouping by entity, like this, could make it easier for newcomers to learn, discover, and quickly add new components.

/xforms-engine
|---/src
|---index.ts
|---hierarchy.ts

|---|---/errors
|---|---|---< common-thing >Error.ts

|---|---/controls
|---|---|---/select
|---|---|---|---SelectNode.ts
|---|---|---|---SelectState.ts
|---|---|---|---SelectControl.ts
|---|---|---|---SelectCodec.ts
|---|---|---|---Select< thing >.ts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this idea, and I've filed #282 to track it. In reality, I think we'll find it's actually a pretty big task, in terms of modeling, naming, and careful consideration of how we apply the concept to internals. I'm not sure whether it could work given the domain concepts and how we model them. But it's a technique for code organization I really like where I've seen it applied (and one we definitely should discuss for the UI package!).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you!

Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { createSharedNodeState } from '../lib/reactivity/node-state/createShared
import { createFieldHint } from '../lib/reactivity/text/createFieldHint.ts';
import { createNodeLabel } from '../lib/reactivity/text/createNodeLabel.ts';
import type { SimpleAtomicState } from '../lib/reactivity/types.ts';
import type { SelectType } from '../parse/body/control/select/SelectDefinition.ts';
import type { SelectType } from '../parse/body/control/SelectControlDefinition.ts';
import type { Root } from './Root.ts';
import type { ValueNodeStateSpec } from './abstract/ValueNode.ts';
import { ValueNode } from './abstract/ValueNode.ts';
Expand Down
2 changes: 1 addition & 1 deletion packages/xforms-engine/src/lib/dom/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type {
KnownAttributeLocalNamedElement,
LocalNamedElement,
} from '@getodk/common/types/dom.ts';
import type { SelectElement } from '../../parse/body/control/select/SelectDefinition';
import type { SelectElement } from '../../parse/body/control/SelectControlDefinition.ts';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

Why do we have one for Select, while not for Range, Note, Inputs?
Looking to understand the reason so I can see if I need it for Rank or Geopoint

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this question?

If you're asking why we have a SelectControlDefinition but not an equivalent definition for the others, we do. They're in the same directory this was moved up to.

If you're asking about the SelectElement type, I think it probably exists because the lookup for selects is a little bit more involved than for other control types (and that would be because of the historical design mistake that we handle <select> and <select1> with the same definitions/node type/implementation).


const hintLookup = new ScopedElementLookup(':scope > hint', 'hint');
const itemLookup = new ScopedElementLookup(':scope > item', 'item');
Expand Down
8 changes: 4 additions & 4 deletions packages/xforms-engine/src/parse/body/BodyDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { ControlDefinition } from './control/ControlDefinition.ts';
import { InputControlDefinition } from './control/InputControlDefinition.ts';
import { RangeControlDefinition } from './control/RangeControlDefinition.ts';
import { RankControlDefinition } from './control/RankControlDefinition.ts';
import type { AnySelectDefinition } from './control/select/SelectDefinition.ts';
import { SelectDefinition } from './control/select/SelectDefinition.ts';
import type { AnySelectControlDefinition } from './control/SelectControlDefinition.ts';
import { SelectControlDefinition } from './control/SelectControlDefinition.ts';
import { TriggerControlDefinition } from './control/TriggerControlDefinition.ts';
import { UploadControlDefinition } from './control/UploadControlDefinition.ts';
import { LogicalGroupDefinition } from './group/LogicalGroupDefinition.ts';
Expand All @@ -24,7 +24,7 @@ export interface BodyElementParentContext {

// prettier-ignore
export type ControlElementDefinition =
| AnySelectDefinition
| AnySelectControlDefinition
| InputControlDefinition
| RangeControlDefinition
| RankControlDefinition
Expand All @@ -48,7 +48,7 @@ const BodyElementDefinitionConstructors = [
PresentationGroupDefinition,
StructuralGroupDefinition,
InputControlDefinition,
SelectDefinition,
SelectControlDefinition,
Copy link
Member

@lognaturel lognaturel Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this will expand to something like ItemBasedControlDefinition with the addition of rank?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that becoming an abstract base class for select/select1/rank. Both concrete classes would still be enumerated here though, as it's used for instantiation during parsing. And the base class would make even more sense when we split select/select1.

RangeControlDefinition,
RankControlDefinition,
TriggerControlDefinition,
Expand Down
13 changes: 7 additions & 6 deletions ...rse/body/control/select/ItemDefinition.ts → .../src/parse/body/control/ItemDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { getValueElement, type ItemElement } from '../../../../lib/dom/query.ts';
import { ItemLabelDefinition } from '../../../text/ItemLabelDefinition.ts';
import type { XFormDefinition } from '../../../XFormDefinition.ts';
import { BodyElementDefinition } from '../../BodyElementDefinition.ts';
import type { AnySelectDefinition } from './SelectDefinition.ts';
import type { ItemElement } from '../../../lib/dom/query.ts';
import { getValueElement } from '../../../lib/dom/query.ts';
import { ItemLabelDefinition } from '../../text/ItemLabelDefinition.ts';
import type { XFormDefinition } from '../../XFormDefinition.ts';
import { BodyElementDefinition } from '../BodyElementDefinition.ts';
import type { AnySelectControlDefinition } from './SelectControlDefinition.ts';

export class ItemDefinition extends BodyElementDefinition<'item'> {
override readonly category = 'support';
Expand All @@ -13,7 +14,7 @@ export class ItemDefinition extends BodyElementDefinition<'item'> {

constructor(
form: XFormDefinition,
override readonly parent: AnySelectDefinition,
override readonly parent: AnySelectControlDefinition,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

I'm supposing that rank will use this, too. Does it make sense?

override readonly parent: AnySelectControlDefinition | RankControlDefinition,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another good catch! Yeah, this module is moving up out of the select directory because I expect it to be used for rank too. And yeah, at a glance that annotation looks right. I'm not sure if we want to introduce some kind of common base class between them.

For what it's worth, I'm not sure the parent property really gets much use. So this type is really only guarding which controls can construct it (which is still valuable IMO).

element: ItemElement
) {
const valueElement = getValueElement(element);
Expand Down
19 changes: 10 additions & 9 deletions .../body/control/select/ItemsetDefinition.ts → ...c/parse/body/control/ItemsetDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { getValueElement, type ItemsetElement } from '../../../../lib/dom/query.ts';
import { ItemsetNodesetExpression } from '../../../expression/ItemsetNodesetExpression.ts';
import { ItemsetValueExpression } from '../../../expression/ItemsetValueExpression.ts';
import { ItemsetLabelDefinition } from '../../../text/ItemsetLabelDefinition.ts';
import type { XFormDefinition } from '../../../XFormDefinition.ts';
import { parseNodesetReference } from '../../../xpath/reference-parsing.ts';
import { BodyElementDefinition } from '../../BodyElementDefinition.ts';
import type { AnySelectDefinition } from './SelectDefinition.ts';
import type { ItemsetElement } from '../../../lib/dom/query.ts';
import { getValueElement } from '../../../lib/dom/query.ts';
import { ItemsetNodesetExpression } from '../../expression/ItemsetNodesetExpression.ts';
import { ItemsetValueExpression } from '../../expression/ItemsetValueExpression.ts';
import { ItemsetLabelDefinition } from '../../text/ItemsetLabelDefinition.ts';
import type { XFormDefinition } from '../../XFormDefinition.ts';
import { parseNodesetReference } from '../../xpath/reference-parsing.ts';
import { BodyElementDefinition } from '../BodyElementDefinition.ts';
import type { AnySelectControlDefinition } from './SelectControlDefinition.ts';

export class ItemsetDefinition extends BodyElementDefinition<'itemset'> {
override readonly category = 'support';
Expand All @@ -19,7 +20,7 @@ export class ItemsetDefinition extends BodyElementDefinition<'itemset'> {

constructor(
form: XFormDefinition,
override readonly parent: AnySelectDefinition,
override readonly parent: AnySelectControlDefinition,
element: ItemsetElement
) {
super(form, parent, element);
Expand Down
18 changes: 9 additions & 9 deletions ...e/body/control/select/SelectDefinition.ts → ...e/body/control/SelectControlDefinition.ts
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

Does body parsing happen after the engine's client? I mean, the entry point is client then body parsing runs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully you've had a chance to read the WIP architecture docs I pushed up EOW, which I think addresses this much better than I can here.

Briefly, the current parsing order is roughly:

  1. Parse XML (currently into a browser DOM tree, subject to change)
  2. Parse jr: URLs to e.g. external secondary instances
  3. Fetch and parse external secondary instances
  4. Parse body
  5. Parse model (binds, primary instance tree into node definitions, translations)

A client calls the engine's initializeForm to initiate this. Once all of that parsing is complete, it's used to build the primary instance state which is then returned to the client (well, resolved in a Promise, because step 3 does IO). We don't (currently) expose any parsing interfaces or logic to a client, it's all encapsulated in that initializeForm call.

Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import type { CollectionValues } from '@getodk/common/types/collections/CollectionValues.ts';
import type { LocalNamedElement } from '@getodk/common/types/dom.ts';
import { getItemElements, getItemsetElement } from '../../../../lib/dom/query.ts';
import type { XFormDefinition } from '../../../XFormDefinition.ts';
import type { SelectAppearanceDefinition } from '../../appearance/selectAppearanceParser.ts';
import { selectAppearanceParser } from '../../appearance/selectAppearanceParser.ts';
import type { AnyBodyElementDefinition, BodyElementParentContext } from '../../BodyDefinition.ts';
import { ControlDefinition } from '../ControlDefinition.ts';
import { getItemElements, getItemsetElement } from '../../../lib/dom/query.ts';
import type { XFormDefinition } from '../../XFormDefinition.ts';
import type { SelectAppearanceDefinition } from '../appearance/selectAppearanceParser.ts';
import { selectAppearanceParser } from '../appearance/selectAppearanceParser.ts';
import type { AnyBodyElementDefinition, BodyElementParentContext } from '../BodyDefinition.ts';
import { ControlDefinition } from './ControlDefinition.ts';
import { ItemDefinition } from './ItemDefinition.ts';
import { ItemsetDefinition } from './ItemsetDefinition.ts';

Expand All @@ -22,12 +22,12 @@ const isSelectElement = (
return selectLocalNames.has(localName as SelectType);
};

export class SelectDefinition<Type extends SelectType> extends ControlDefinition<Type> {
export class SelectControlDefinition<Type extends SelectType> extends ControlDefinition<Type> {
static override isCompatible(localName: string, element: Element): boolean {
return isSelectElement(element, localName);
}

static isSelect(element: AnyBodyElementDefinition): element is AnySelectDefinition {
static isSelect(element: AnyBodyElementDefinition): element is AnySelectControlDefinition {
return selectLocalNames.has(element.type as SelectType);
}

Expand Down Expand Up @@ -72,4 +72,4 @@ export class SelectDefinition<Type extends SelectType> extends ControlDefinition
}
}

export type AnySelectDefinition = SelectDefinition<SelectType>;
export type AnySelectControlDefinition = SelectControlDefinition<SelectType>;
21 changes: 0 additions & 21 deletions packages/xforms-engine/src/parse/body/control/select/ItemsetNodesetContext.ts
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion packages/xforms-engine/src/parse/expression/ItemsetNodesetExpression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { ItemsetDefinition } from '../body/control/select/ItemsetDefinition.ts';
import type { ItemsetDefinition } from '../body/control/ItemsetDefinition.ts';
import { DependentExpression } from './abstract/DependentExpression.ts';

export class ItemsetNodesetExpression extends DependentExpression<'nodes'> {
Expand Down
2 changes: 1 addition & 1 deletion packages/xforms-engine/src/parse/expression/ItemsetValueExpression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { ItemsetDefinition } from '../body/control/select/ItemsetDefinition.ts';
import type { ItemsetDefinition } from '../body/control/ItemsetDefinition.ts';
import { DependentExpression } from './abstract/DependentExpression.ts';

export class ItemsetValueExpression extends DependentExpression<'string'> {
Expand Down
2 changes: 1 addition & 1 deletion packages/xforms-engine/src/parse/text/ItemLabelDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { LocalNamedElement } from '@getodk/common/types/dom.ts';
import { getLabelElement } from '../../lib/dom/query.ts';
import type { XFormDefinition } from '../../parse/XFormDefinition.ts';
import type { ItemDefinition } from '../body/control/select/ItemDefinition.ts';
import type { ItemDefinition } from '../body/control/ItemDefinition.ts';
import { TextElementDefinition } from './abstract/TextElementDefinition.ts';

interface LabelElement extends LocalNamedElement<'label'> {}
Expand Down
4 changes: 2 additions & 2 deletions packages/xforms-engine/src/parse/text/ItemsetLabelDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type { LocalNamedElement } from '@getodk/common/types/dom.ts';
import { getLabelElement } from '../../lib/dom/query.ts';
import type { XFormDefinition } from '../../parse/XFormDefinition.ts';
import type { ItemDefinition } from '../body/control/select/ItemDefinition.ts';
import type { ItemsetDefinition } from '../body/control/select/ItemsetDefinition.ts';
import type { ItemDefinition } from '../body/control/ItemDefinition.ts';
import type { ItemsetDefinition } from '../body/control/ItemsetDefinition.ts';
import { TextReferenceExpression } from '../expression/TextReferenceExpression.ts';
import { TextTranslationExpression } from '../expression/TextTranslationExpression.ts';
import type { RefAttributeChunk } from './abstract/TextElementDefinition.ts';
Expand Down
2 changes: 1 addition & 1 deletion packages/xforms-engine/src/parse/text/abstract/TextElementDefinition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { isElementNode, isTextNode } from '@getodk/common/lib/dom/predicates.ts';
import type { ElementTextRole } from '../../../client/TextRange.ts';
import type { XFormDefinition } from '../../../parse/XFormDefinition.ts';
import type { ItemDefinition } from '../../body/control/select/ItemDefinition.ts';
import type { ItemDefinition } from '../../body/control/ItemDefinition.ts';
import { TextLiteralExpression } from '../../expression/TextLiteralExpression.ts';
import { TextOutputExpression } from '../../expression/TextOutputExpression.ts';
import { TextReferenceExpression } from '../../expression/TextReferenceExpression.ts';
Expand Down