Support for <select> and <select1> value types, partial support for <range>
#273
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These were originally implemented against an interface when it wasn’t clear how similar they would be, or how stable. It turns out: both, quite a bit! And the more complex property types didn’t really add much value, but did make things harder to reason about directly. Refactoring all NodeDefinition implementations to use this new shared abstract base class will help consolidate common changes among them.
Includes `NoteCodec`, a wrapper over any base `ValueCodec` ensuring blank values are always represented as `null`. While a minor variation on the shared codecs, this demonstrates how `ValueNode` is designed to be completely generic over a `ValueCodec` its and runtime type representations.
…e type The idea here is that we’ll want notes with values to render the note’s runtime value representation as text, which we anticipate being different from its `instanceValue` state! It’s set up to be type safe as we expand the set of supported value types, ensuring we’ll have to make updates when we do expand those types to serialize e.g. geo values to human-readable text.
The engine-internal `RangeControl` implementation is very similar to the `int`/`decimal` value type cases of `InputControl`. These are the most important substantive differences: 1. We parse `min`/`max`/`step` properties (from `start`/`end`/`step`, respectively) from the body element. These are parsed into the same runtime value representation as the node’s value state. 2. The `min` and `max` bounds are enforced. This is handled in `RangeCodec`, which composes the base int/decimal codecs. 3. We also provide range-specific `appearance` type hints (reminder that these _are only type hints!_, we always parse all appearances regardless of the ones we hint). - - - Note that `step` is currently **NOT** enforced. It seems pretty likely it should be. I held off here because implementing that _kind of_ suggests a slightly different API. Namely, it might make sense to provide the available steps in `RangeNode.currentState.valueOptions`. I held off here because the current `BaseValueNode`/`ValueNode` types do not deal with `valueOptions` at all. It’s currently hard-coded to `null` for all descendant node types, although there isn’t any logic which necessitates it. I chose that as a simplification as I anticipate some churn around other nodes with `valueOptions` (i.e. selects).
This change ensures each concrete subclass of `PositionalEvent` can only be instantiated by _its own static `from` method_ with the same node type its (generally non-public) constructor accepts. (I am addressing this here because I was surprised not to get type errors with the engine’s introduction of range nodes.)
To my slight surprise, no tests are affected! I’ll add some coverage in a followup commit.
Supported:
- Base/default UI to select a range value with a slider
- Values are constrained by `min`/`max`/`step`
- appearance: `vertical`
Not yet supported:
- Ticks (see commentary in the `RangeControl` component)
- Indication of the presently selected value (see commentary there as well); any consideration of presenting intermediate values
- appearances:
- `picker`
- `rating`
I held off on both of the pending appearnaces to keep scope of this commit down. I also presume we’ll want to put some thought into UX design, especially for `picker`.
This is preparation for moving `SelectNode` over to the `BaseValueNode`/`ValueNode` abstractions. `RangeNodeState` is updated to make the previous default explicit.
This change: 1. Has immediate ergonomic benefits, addressing integration challenges which arose integrating the original `select`/`deselect` methods. These benefits will be immediately clear in the following commits updating client integrations. 2. Lays groundwork for incorporating bind/data type support in select nodes. - - - Tangentially, this change also more accurately reflects the type of `SelectItem.label`, which will also simplify client integrations of same.
… and non-nullable `SelectItem.label`.
…de setters… … and non-nullable `SelectItem.label`. Here the benefits of these API refinements should be most obvious. This commit also expands test coverage to broadly exercise each integration with the new setter methods. This will be valuable to ensure each affected component is not broken as we make other revisions to `SelectNode`.
This is a bit of a wash in terms of client ergonomics, in some ways an improvement and in others a bit fussier. But it’s an important piece of groundwork for `SelectNode` to support other bind/data/value types. - - - Worth considering for the future: maybe it would make more sense to consolidate: - `SelectNode.currentState.valueOptions` - `SelectNode.currentState.value` … such that there is a single list, with the actual _selected_ value state represented on individual `SelectItem`s? This seems like it would vastly improve client ergonomics. I’m making note of it here because it would run into the same problems with the `ValueNode` abstraction which precipitated this change. This is a class constructor chicken/egg problem, where: 1. The `ValueNode` base class constructor expects a `codec` parameter, handling mapping between instance state (the string value of the node) and the node’s runtime reprsentation. 2. The `SelectItem[]` representation is (typically) dependent on XPath evaluation (of itemsets, even static item labels). The `SelectField` (to be renamed `SelectControl` in incoming commit) instance cannot be referenced for (2) (as `this`) until the `super(…)` call in (1) is complete. There are ways around this, but they’re all… full of compromises and shenanigans that I’d expect to be hard on maintenance.
…ngs … … vs `SelectItem`s. I **believe** expanded test coverage caught all of the mistakes I made in the course of making this change. My reduced confidence essentially boils down to poor type safety: - In PrimeVue’s own provided types; which is exacerbated by… - Many of those types being effectively reduced by our own type augmentations to little more than `Record<string, any>`[^1] We _really_ need to improve these types to have more confidence in changes like this. I can make similar changes to `scenario` figuratively “in my sleep”, because type checking catches ~everything I might miss (and anything else is caught by `scenario` being a test suite). [^1]: The issue is that our type augmentations are _not really augmentations_, they’re overriding PrimeVue’s built-in types. Each `.d.ts` file which should be augmenting something from PrimeVue would need to have some sort of top-level import or export to fix this. I briefly explored making that change, but it revealed that some of our agumentations are _incompatible_ (albeit more correct!) with the underlying types from PrimeVue. I’m not sure how to best solve this, but I strongly suspect it will involve either wrapping PrimeVue components where we use them, or contributing type improvements upstream. (The latter being preferable anytime we’d augment any third party types, at the expense of coordination with other projects to make progress.)
🦋 Changeset detectedLatest commit: a0c3da2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
eyelidlessness
force-pushed
the
features/value-nodes
branch
from
c5cb8a1 to
4c94f85
Compare
eyelidlessness
changed the title
<select> and <select1> value types, partial support for <odk:range>
eyelidlessness
commented
Jan 14, 2025
| } | ||
|
|
||
| export interface SelectDefinition extends LeafNodeDefinition { | ||
| export interface SelectDefinition<V extends ValueType = ValueType> extends LeafNodeDefinition<V> { |
There was a problem hiding this comment.
I would like to think about this naming convention. Note that this currently conflicts with the definition for its bodyElement, which follows an earlier naming convention, and I definitely would prefer renaming that regardless of the convention for node definitions.
I'd also really like to consider removing node definitions from the engine/client API entirely. I don't think they've ever provided much value, and I continue to chip away at what little they do provide whenever I get a chance.
lognaturel
reviewed
Jan 14, 2025
There was a problem hiding this comment.
The range implementation looks like a great start.
I'm getting turned around in my understand of ValueNode. When I reviewed its introduction, I saw it mostly as a way to standardize how values were converted to and from strings through the codec system. Now that I review the commit notes, I see that type parameterization was also a significant consideration.
I see the value of type parameterization for range which has int and decimal variants.
If I'm not mistaken, trigger is migrated to use ValueNode mostly to take advantage of the codec system, am I understanding correctly?
note is a fuzzy concept since it's not in ODK XForms. XLSForms introduces it as a read-only string node. Collect doesn't explicitly have a note concept but it has special ways of displaying read-only nodes of any type. The parameterized typing for note makes sense to me to match that Collect behavior.
Selects are defined in ODK XForms as bound to strings (https://getodk.github.io/xforms-spec/#body-elements) and I don't think we should expand on that. People who define forms can (and do!) use other types as the underlying item values and then can use them as those types in downstream calculations. The downside of course is that there's nothing validating that they're consistently using the same type when defining their choices but as far as I know this has not been a significant source of problems. My understanding is that there's still value to using ValueNode for the codec machinery, am I getting that right? Is the eventual intention to have all all node types extend from BaseValueNode?
| } | ||
| } | ||
|
|
||
| export type AnyNote = |
There was a problem hiding this comment.
This is because WF defines a note as any node that is read-only and has no value? It makes me nervous that these are literal string types as opposed to some kind of reference. Presumably this is because there's no way to define types based on a collection? There's nothing that's going to help us remember to update this as types are added, is there?
There was a problem hiding this comment.
I think this is mostly addressed at 43cc144
| }; | ||
|
|
||
| const multipleValueSelectCodecs: MultipleValueSelectCodecs = { | ||
| string: new MultipleValueSelectCodec(sharedValueCodecs.string), |
There was a problem hiding this comment.
There is no expectation of selects working for any bind type other than string. You can see at https://getodk.github.io/xforms-spec/#body-elements that data type string is specified
| * As the name implies, this is not ported from JavaRosa. It is a proposed | ||
| * addition, for parity with {@link select1}. | ||
| */ | ||
| export const proposed_select = (ref: string, ...children: XFormsElement[]): XFormsElement => { |
Client interface: - `SelectNode` is now generic over its `valueType`, as with other node types now supporting value types. - The `isSelected` method is added as a convenience for clients, as the underlying logic would be fussy and error prone if clients were required to implement it. Engine internal—`SelectControl`: - `SelectField` is renamed to `SelectControl`, consistent with other `ValueNode` concrete node implementations - `SelectControl` is a concrete `ValueNode` implementation - `SelectControl` implements `SelectNode`’s generic `valueType`, detailed below Engine internal—`ValueArrayCodec`, `SingleValueSelectCodec`, `MultipleValueSelectCodec`: - A generalized base class `ValueArrayCodec` expands on the `ValueCodec` concept to address values with a runtime representation as an array of values. This class composes a base codec, and (at least presently) is expected to produce arrays of values of _almost_ the same type as their single-value counterparts (e.g. as represented for `InputControl`). The main distinction is that `ValueArrayCodec`’s decoder is expected to omit `null` values as array items. This is presumably suitable for all current controls with value options (including `<select>`/`<select1>`, but also anticipated support for `<odk:rank>`), as it seems unlikely we’d support `null` value options for any data type (where `null` is used as a representation of the “blank” value for any given data type) - `MultipleValueSelectCodec` essentially implements value types by mapping a base codec over a list of whitespace-separated substring values - `SingleValueSelectCodec` applies the same composition, but does not treat the instance value as a whitespace-separated list; instead, a blank value produces an empty array, and a non-blank value is treated as a single array item - - - Note that this change: 1. Fixes the outstanding issue with `<select1>` values incorrectly sharing whitespace logic which should only apply to `<select>`. 2. Does _not_ address the awkwardness of a single node type (`SelectNode`) representing both controls. I briefly explored addressing that in this change, but found the scope hard to manage. This change is also intended to lay groundwork and establish a better frame for reference for work to support `<odk:rank>`. It is expected that all of the following will be pertinent references for that work: - `SelectNode` (client interface) - `SelectControl` (internal implementation of that interface) - `ValueArrayCodec` (base) and `MultipleValueSelectCodec` (concrete) In fact, I wouldn’t be surprised if the codec logic for a hypothetical `RankControl` implementation would be identical, and we may want to discuss a more general naming scheme. The most significant _semantic differences_ we expect are that `<odk:rank>` values: - should include all available values - should produce values in their written order, rather than the order they appear in value options (e.g. from the form definition’s `<item>`s or the effective `<itemset>` node-set)
Note that this doesn’t _currently_ do anything special for individual value types! I think that’s expected for the subset of value types we presently implement (which are all represented as primitive values, and all presentable in the current UI controls the same way string values have been). That will necessarily change as we add support for more complex value type representations (e.g. geo).
eyelidlessness
force-pushed
the
features/value-nodes
branch
from
1769c2a to
a0c3da2
Compare
lognaturel
changed the title
<select> and <select1> value types, partial support for <odk:range><select> and <select1> value types, partial support for <range>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Expanded test coverage (especially of selects).
Why is this the best possible solution? Were any other approaches considered?
Select (and note) value types
This brings
SelectNode(andNoteNode) in line with other controls supporting value types. As much as reasonable, those changes are relatively direct and minimal. There are some changes to theSelectNodeclient-facing write APIs which have been long discussed, and for which this was an excellent opportunity. It also made it much more straightforward to complete the change (more minimally than preserving the less ideal write APIs).Especially for selects, I used this change as additional validation that the underlying abstractions/concepts are both appropriate and well suited for all value node types. I was unsure of that through the development process, but ultimately concluded that the foundation is solid.
Range control
It would be worth considering other implementations of the basic control. Notably, the PrimeVue component does not support ticks.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
Fixtures are added for range controls, as well as for typed selects.
What's changed
This is admittedly too large in scope! It came out of a strategy discussion with @lognaturel before EOY 2024, where we discussed how I could best be productive while ODK would be slow over the holidays, as I was expecting I'd still have some motivation to move the project forward during that time. It further evolved as we discussed laying groundwork for
<odk:rank>functionality. I think the individual commits are fairly representative of the individual changes. Here I'll call out some details I think are worth extra attention.First, the initial commit (6a68552) isn't especially important in its own right, and I don't believe it's strictly necessary for the rest of the changes. I expected to leverage this change more when I started this journey. Ultimately I decided to keep it in anyway, because it lays groundwork for other changes we'll benefit from later (explored at least in the context of perf optimization).
Migration of most value/leaf nodes to
BaseValueNode(client interface),ValueNode(internal)This change migrates all of the following:
TriggerNode/TriggerControlNoteNode/NoteSelectNode/SelectFieldSelectControl(more detail below)RangeNode/RangeControl: partial support for<range>controlsThis change introduces partial support for
<range>controls.Not yet supported:
appearance="no-ticks": this is currently the default behavior, but it's not explicitly handled, so I think it's a stretch to say it's complete.appearance="rating"appearance="picker"SelectNode/SelectControlsupport for value typesNote:
SelectFieldhas been renamed toSelectControl, which is consistent with the engine's internal implementation of other control node types.This fits the general theme of migrating existing leaf/value node types to the underlying
BaseValueNode/ValueNodeabstractions. I'm calling it out separately here because it brings with it some breaking changes, and because the change itself is more involved.Breaking changes: the
SelectNodewrite APIs have been revised to be more ergonomic for clients, and to accept values rather thanSelectItems. AselectValuemethod is added for setting a single value (most useful for<select1>). AselectValuesmethod is added for setting all of the node's values (most useful for<select>).There are also some additive changes to bolster the improved ergonomics of mapping between select values and
SelectItems.Future considerations: I decided not to address the other elephant in the room with the engine's design for selects. Namely, this change does not break out distinct node types for
<select1>and<select>. I feel strongly that we should do this, but it would have blown up scope. (I briefly explored it hoping it would actually simplify the change, but ultimately found it hard to keep track of everything.)SelectControl's use ofValueCodecTo support migrating
SelectControlto theValueNodebase class, it was necessary to implement a correspondingValueCodec. This is accomplished with:ValueArrayCodec: an abstract base for representing an array of values (more on this below)MultipleValueSelectCodec: corresponds to<select>. This effectively maps a baseValueCodec's functionality over an array of string values (splitting a whitespace-separated list from the form definition)SingleValueSelectCodec: corresponds to<select1>. This similarly applies a baseValueCodec's functionality, but special cases the control's single value semantics. This has two important benefits:<select1>values.ValueCodecfor allSelectControls of the samevalueType, but I found it caused a significant performance regression on all of the forms I check for performance. (I didn't even get as far as checking! I was just surprised to see some of the bigger Scenario tests complete much more slowly.)SelectControl,ValueArrayCodec(andMultipleValueSelectCodec) as foundational for<odk:rank>The intent of bringing selects into the scope of this grab bag was to lay foundation for
<odk:rank>controls. Making theValueNodechange now felt important as I expectSelectNodeandSelectControlwill be quite similar to their rank counterparts, with the latter's semantics nearly totally overlapping the semantics of<select>.I expect that
ValueArrayCodecwill also serve as a basis for that work. I strongly suspect the actual concrete implementation will be identical toMultipleValueSelectCodec, so it's quite likely that class will have its name changed in the near future.I've also tried to leave a few other breadcrumbs in comments and/or commit notes, where I think the subtle nuances between the controls might come into play.