Skip to content

Form definition, bindings & computations: first pass #9

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.

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

Merged
merged 20 commits into from
Nov 17, 2023

Conversation

eyelidlessness
Copy link
Contributor

This PR is a first pass on these aspects of XForm functionality:

  • A runtime representation of a form definition (XFormDefinition and several downstream types) including:
    • Some form-global metadata like title
    • Explicit nodeset bindings and their computation expressions (calculate, readonly, relevant, required)
  • A corresponding reactive representation of entry state derived from said definition (XFormEntry)
  • Basic support for groups to introduce additional UI support for some aspects of the above

The bulk of the code in the PR is in the form definition representation, whereas the bulk of the user-facing benefit is in the comparably small reactive portion derived from that.

All of this apart from some refinement of groups was demoed in our weekly call on Tuesday, and I believe @lognaturel has already looked at the code in some detail, so I expect there's little surprise on that front. I will highlight a couple of things that I think are worth noting here or may be important points of reference for future work:

  1. While the share of reactive code is small, of course it's backed by Solid's reactivity. Besides the fact that it does a bunch of stuff for us that we don't have to worry about (yay!), a lot of the reactive portion of the code is much more heavily commented than I tend to prefer. Partly this is because I'm still learning Solid's reactive primitives, but mostly it's because they come with quite a bit of nuance and consideration for such a small API. I also want to explicitly call this out because I expect churn around the reactive bits.
  2. I've already mentioned to @lognaturel that I'm not particularly married to many of the interfaces introduced here. Some of them reflect how I was already thinking in earlier prototyping, some are directly inspired by cursory exploration of JavaRosa, and some are pretty off the cuff.
  3. Of those which are more off the cuff, one area of abstraction feels particularly strong (and I expect will inform several areas with similar domain shape): BindExpression (and its subclasses) and createBindExpressionEvaluation. Branches downstream from this one are already "begging" for a similar set of types and reactive setup. I don't want to overthink this too soon, but I suspect there may fall out an even more general base case which could serve many aspects of engine computation.

I feel like there's probably more I should write up here, but I'm blanking now and don't want to hold up the PR. If I think of more that belongs in this summary I'll come back and edit.

This doesn’t really introduce new functionality, but it starts to build out a structure that will begin to support a fuller form type/definition
…luator

This will support sharing the same parser (which caches parse trees, and simplifies tree-sitter objects) for e.g. determining sub-expression dependencies in XForms computations
… and one field’s default value, both wired up to render (but nothing interactive yet)
Currently `XFormDefinition` is implicitly creating the initial form state, including the naive reactivity used to perform the basic calculate. This (correctly) causes warnings about those reactive calls not being created under a reactive root (because the fetch is async).
- Supports attribute and element nodes
- Does not affect parent elements
- Derives initial state from DOM node value (element’s text content, attributes value)
- Updates DOM node value from setter call
- Provides setter interface consistent with normal Solid signal setter (i.e. accepts a value, or a callback with the signal’s current value as its parameter)
This does not (and wasn’t yet intended to) work for reacting do dependency changes.

Well… at least not in the view. Somehow it is recalculating the serialized submission state. Given the fact that I already had to wrap a binding access in `createMemo`
- calculate
- readonly
- relevant
- required

Includes some notes on bugs and incomplete functionality. Mostly not tested at the view level, but not a lot of view logic has been introduced (which I’m hoping hints at how this separation can be kept as the engine logic progresses).

Also includes some basic demo forms exercising these computations
…s restored

Also properly ensures calculations are not performed when non-relevant
Some of the way labels are handled feels a little bit convoluted *now*, but the intent is to share logic even as rendering may become more compicated (i.e. supporting translations and outputs)
- Collapse icon is vertically aligned with first text line
- Text alignment is inherited from its context (this hasn’t been tested for RTL, but it will likely work as expected)
Also updates JSDoc for the binding state interface to clarify each member’s relationship to the underlying XForm specs, and certain nuances of state behavior.

The logic for readonly inheritance is remarkably similar to that of relevant, but different enough to warrant repetitiveness IMO. Particularly because these are the only inheritance cases specified in the pertinent W3C XForms spec section.
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

🎉

export type XFormViewChildType = SupportedXFormViewChildType | UnsupportedXFormViewChildType;

const viewChildType = (element: Element): XFormViewChildType => {
// TODO: spec includes `odk:rank`, do we check its namespace? All others are
Copy link
Member

Choose a reason for hiding this comment

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

getodk/javarosa#345

In general I've really had trouble deciding how I feel about namespacing. If we had a rich ecosystem of people trying out new ideas and then contributing them to the core, namespaces would be worth the overhead. But we don't so they feel a little silly. My current position is that I don't really care about namespacing for now but if we do end up with more sources of new implementations I'd be open to a more formal approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I'm of two conflicting minds on this (and they probably won't be surprising).

  1. Collect/JavaRosa is the reference implementation, and consistency is key.
  2. The spec is not ambiguous, and conformance is preferred over special cases.

I'm particularly cautious about [either implementation, or both] being internally inconsistent in ways that become harder to support and maintain. (Example: right now, AFAIK @odk/xpath consistently supports namespaces according to spec, and @odk/web-forms has mixed support where it's either ignored or explicitly called out like it is here.)

Pragmatically I'm inclined to agree with your position, in large part because there's a much larger scope of "should" to tackle than this, but in the fullness of time my inclination would be to support namespaces consistently (with spec and internally)—ideally with a design which isn't particularly onerous in the first place, so it becomes more pragmatic to just do it "right" (and then, ideally in every implementation).

* As in ODK XForms Spec.
*
* TODO: it's unclear why JavaRosa's `DataType` hews closely to the spec, but
* has certain differences (e.g. string -> TEXT, int -> INTEGER). It's also
Copy link
Member

Choose a reason for hiding this comment

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

I feel like I left this comment somewhere but I'm not finding it. Sorry if I'm repeating myself! I would not read anything into the constant names.

* TODO: it's unclear why JavaRosa's `DataType` hews closely to the spec, but
* has certain differences (e.g. string -> TEXT, int -> INTEGER). It's also
* not immediately clear how additive types like CHOICE and MULTIPLE_ITEMS
* square with the underlying spec types.
Copy link
Member

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. select ones and select multiples are string values. The fact that they're displayed as lists of options is only defined in the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm referring to DataType's CHOICE and MULTIPLE_ITEMS enum values, which go beyond the STRING type I'd have expected.

{ expression: '/foo/bar', expected: ['/foo/bar'] },
{ expression: 'if(/foo = true(), /bar, /quux)', expected: ['/foo', '/bar', '/quux'] },
{
expression: 'coalesce(., id("ent")/ity, id("ent"))',
Copy link
Member

Choose a reason for hiding this comment

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

I had a comment somewhere else about id, right? It's not used in the ODK XForms subset. I don't think it matters for these kinds of tests but wanted to make sure it's mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall another comment, this is good to know. I think it's pretty likely this won't even last long in whatever evolves from the functionality tested here anyway.


// TODO: it is unclear whether this will need to be supported.
// https://github.com/getodk/collect/issues/3758 mentions deprecation.
readonly saveIncomplete: BindExpression | null;
Copy link
Member

Choose a reason for hiding this comment

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

Enketo hasn't historically supported saveIncomplete. It's a really weird one we've almost deprecated a number of times but haven't because analytics show us it's in use. Basically it's for very long forms or forms that call out to external apps. It lets the form designer explicitly say they want the system to save a draft record rather than a recovery savepoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense and lines up with my (er) incomplete reading of the background. I think it would make sense to treat this as "to align, later if/when cycles allow", and maybe attach some sort of a warning to it until then (not in this PR, but maybe whenever I come upon this TODO again in a future pass)?

// deferring final constraint validation until submission time or some other
// event which triggers a more thorough recomputation, but I don't believe that
// is strictly necessary (nor necessarily a significant performance concern).
readonly constraint: BindExpression | null;
Copy link
Member

Choose a reason for hiding this comment

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

I believe constraints are not in the DAG so that self-references can always be rejected as form design errors. I'm really not convinced that's the right thing to do, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. In earlier prototyping, I had included constraint dependencies, and special cased it to exclude self references. That at least fits my mental model of the spec intent.

state = baseState;
} else {
// As the name states, calculations are only performed when a question is
// relevant. Returns the current state otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Is the base state a blank value? That's what it should be, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're referring to the signal getter named baseState (which deserves a better name...), not exactly. The state is retained in memory separately from the DOM state of the entry/instance, so it might be restored when the node becomes relevant. When non-relevant, the DOM node value is set to blank but the in-memory state is preserved. I believe this is consistent with the [intended] behavior in Enketo with excludeNonRelevant, which I believe is consistent with the same behavior in Collect/JR.

It probably also bears clarifying that the DOM state is the source of truth for:

  • entry/submission (i.e. what's in the DOM is what will be submitted when the user clicks "Send")
  • XPath evaluation (i.e. blank in the DOM = blank in evaluation of expressions referencing the DOM node)

The in-memory state is responsible for:

  • interfacing between the view and the entry/submission DOM state (i.e. reflecting DOM state when relevant, persisting state changes to DOM)
  • facilitating reactivity between dependent nodes when their dependencies change (i.e. recomputing calculate/relevant etc, propagating state changes to the DOM in turn)

Some scenarios to illustrate (and if these help, maybe it would also help to bring them into comments, though I'll caution that I expect implementation detail changes here which might invalidate some or all of this).

Given this form

<h:html>
  <h:head>
    <model>
      <root>
        <foo>a</foo>
        <bar>b</bar>
        <quux />
      </root>
      <bind nodeset="/root/foo" calculate="/root/bar" relevant="/root/quux != 'c'" />
      <bind nodeset="/root/bar" />
      <bind nodeset="/root/quux" />
    </model>
  </h:head>
  <h:body>
    <input ref="/root/bar" />
    <input ref="/root/quux" />
  </h:body>
</h:html>
Scenario foo relevant? <foo> value (DOM, entry, submission) baseState()
0. Form definition construction yes <foo>a</foo> 'a'
1. State init1 yes <foo>b</foo> 'b'
2. User sets quux to 'c' no <foo /> 'b'
3. User clears quux yes <foo>b</foo> 'b' (currently recalculates, but value is unchanged)
4. User sets foo manually to 'u' yes <foo>u</foo> 'u'
5. User sets quux to 'c' no <foo /> 'u'
6. User sets bar to 'x' no <foo /> 'u' (calculation deferred while non-relevant)
7. User clears quux yes <foo>x</foo> 'x' (calculation no longer deferred)
8. User sets foo manually to 'z' yes <foo>z</foo> 'z'
9. User sets quux to 'c' no <foo /> 'z'
10. User clears quux yes <foo>z</foo> 'z' (calculation skipped, user input was more recent than calculate dependency change)

Further, state and baseState should produce the same value when read. The reason they're separate is to be able to track the most-recently-written state (manual/user versus calculate/reactive) and know which to restore on re-relevance.

Also, all of this is describing my understanding of the behavior. I believe most of it is covered in tests, but insofar as it isn't it should be, and it's possible the actual behavior differs in ways that either need correction or have some implementation-detail-nuance I may have missed.

Footnotes

  1. This is simplified, as the default value persists temporarily before calculation is evaluated/propagated. This isn't observable from the outside.

* Returns the binding's **runtime value**. This value may differ from its
* state in the submission DOM: if the binding is presently non-relevant, its
* DOM value will be blank but its runtime value will remain set to whatever
* it had been prior to becoming non-relevant.
Copy link
Member

Choose a reason for hiding this comment

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

its runtime value will remain set to whatever it had been prior to becoming non-relevant

Ah, ok, so recalculation happens on relevance change not on value change of a subexpression value? That's a deviation from JR which IIRC continues recomputation of non-relevant values. I think the only difference will be when there's a potential performance hit: at time of a subexpression value changing vs. when relevance changes. Probably not important to align.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that there's no sense recalculating when we know the value is going to be ignored (i.e. the actual submission state for the node will be blank no matter what the calculated value might be). I'm not even sure there is anything to align, so long as the submission state it produces is the same. Unless there's something I'm missing?

* - non-relevant
* - readonly
*
* Instinctually, both should prevent setting any value.
Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, with the exception of actions (setvalue, setgeopoint). I know you're not there yet but something to keep in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is specifically about user-supplied values, so I don't think that'll be an issue. Neither prevent (or would prevent) setting state from calculate, and I'd expect the same to be true for actions.

@eyelidlessness
Copy link
Contributor Author

Discussed feedback followup with @lognaturel on Slack, sounds like we're still good to merge, so here we go!

@eyelidlessness eyelidlessness merged commit 0ff5b4e into main Nov 17, 2023
@eyelidlessness eyelidlessness deleted the xform/initial-form-def-start-engine branch November 17, 2023 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants