Engine support for jr:count and jr:noAddRemove

[eyelidlessness]

As the title says, this PR adds support for jr:count and jr:noAddRemove.

Client-facing changes

In terms of client impact, this change splits the RepeatRangeNode client interface into:

• RepeatRangeControlledNode (nodeType: 'repeat-range:controlled')
• RepeatRangeUncontrolledNode (nodeType: 'repeat-range:uncontrolled')

From a client perspective, the main distinction between these nodes aside from their names and nodeType is that only RepeatRangeUncontrolledNode provides write methods (addInstances, removeInstances).

The client interface still exposes a RepeatRangeNode type, which is now a union of these two interfaces.

An earlier pass considered the possibility of keeping nodeType: 'repeat-range' for both types, with a separate countType property to narrow their capabilities further. And as described in the earliest commits to this branch, the approach works (i.e. type narrowing behaves as expected), but the usage felt confusing and error prone.

I'm eager to bikeshed the names of these types if there's interest, but I'm pretty confident the general approach (distinct nodeTypes as the narrowing mechanism, distinct names for each node type, clear naming corresponding to their distinct capabilities) is the right way to go.

I left the earlier approach intact in the commit history for now (which we may choose to rebase before merge), mainly to address some concern in earlier discussions about encoding more information into the nodeType property. I do understand that from a data modeling perspective this is a bit of a smell! But I really think it's the right way to go. If there are still doubts, let's consider getting on a call to discuss the tradeoffs before digging in here in the comments.

Implementation: RepeatCountControlExpression

A new RepeatCountControlExpression which generalizes the logic of both jr:count and jr:addRemove. This is another DependentExpression, the underlying interface used by reactive createComputedExpression, as well as all of the same dependency analysis code paths used to compute calculate, relevant... any other expression which might update based on changes to dependencies.

This is generalized by treating jr:noAddRemove as if it were a jr:count expression which evaluates to the the number of form-defined repeat instances.

Judgment call: special case for jr:noAddRemove with only a jr:template

If only a jr:template is defined, jr:noAddRemove is treated as if one instance is defined. I'm happy to reconsider this, but it seemed like the first sensible thing I could think of. Maybe we should warn instead?

Implementation: BaseRepeatRange, RepeatRangeControlled, RepeatRangeUncontrolled

Nearly all functionality previously in RepeatRange is moved into an abstract BaseRepeatRange class, from which the controlled and uncontrolled concrete node implementations inherit.

Only the behavior distinct to those node types in those concrete classes. Those differences are:

• RepeatRangeUncontrolled creates initial repeat instances almost the same way RepeatRange did before. (More on that below!)

• RepeatRangeControlled creates instances with a reactive computation of RepeatCountControlExpression, both on initialization and on any subsequent update to referenced dependencies.

• As with the client interface, only RepeatRangeUncontrolled exposes public write methods to add or remove repeat instances.

Judgment call: special case when jr:count produces NaN

Currently, when a jr:count expression is evaluated to NaN, the count of repeat instances is not updated until a subsequent update produces a valid number. I added some commentary on this in 052ffbd. It matched the behavior I expect when I was directly setting a count in an input. But I started to second guess this as I was writing tests around count being computed from multiple/indirect references.

I suspect it might be better if we were to let clients provide a special case for this, but that may require doing additional form analysis so the engine can inform clients when a particular field controls a jr:count expression. Should we maybe roll this special case back and consider refinement in the future?

Change in repeat instance creation logic

There is one important change to the code moved from RepeatRange into BaseRepeatRange. Under some circumstances, synchronous addition of multiple RepeatInstance children is now performed in two (still synchronous) steps:

1. All of the pending RepeatInstances are constructed in memory.
2. Once created, they are all added to the parent repeat range's childrenState in a single write call.

This introduces a brief period where reactive state lags the state in the backing XML DOM store used for XPath evaluation. There are a couple of motivations for this change:

• It's much easier to reason about! The creation code is a simple array map, and there's a single write call. This was previously implemented by recursively calling addInstances, which especially made it confusing to understand how and when that method's default parameters were used.

• It addresses one of the lingering cases where clients using Solid (including scenario) will encounter InconsistentChildrenStateErrors. And I now have a much better understanding of what causes this to occur! Ultimately, this is caused when certain writes, in certain reactive scopes, are issued multiple times in the same synchronous tick. I'm not 100% confident that we've seen the last of these cases, as it could still be produced under some circumstances by repeated calls to RepeatRangeUncontrolled.addInstances.

Tests

There were no existing scenario tests from JavaRosa which passed with these changes. This is only a little surprising, based on my understanding of the history of those tests. There are certainly some tests which will be affected by the introduction of jr:count functionality, but they exercise additional functionality as well.

As such, I've added a slew of new targeted tests for these specific features and various details thereof.

Bonus bug fix: reactive update on indirect reference to non-blank + newly relevant nodes

One of the new scenario tests happened to trigger a bug that was familiar to me, and for which I've had a known fix awaiting prioritization. Specifically, this is the nature of the bug as I understand it:

1. Given some field a with:
   • A non-blank value
   • A relevant expression evaluating to false
2. Given some field b with a computation referencing a
3. Given some field c with a computation referencing b

Making a relevant will sometimes cause updates to b to lag, and will generally (if not always) prevent c from updating a consequence.

The fix for this is simple: f16a4ac. I'll call out here (as I did in a comment there) that this kind of bug will be moot if we move away from XML DOM for XPath evaluation.

Clients

Apart from scenario, I've made minimal updates to the web-forms (Vue UI) and ui-solid clients to account for the changes in the client-facing API. I did not address any presentation aspects of the change in either UI client. I expect there will be some minor adjustments incoming (probably at least around spacing), but I've deferred to downstream work on that.

Open questions

There are a few open questions raised above (name bikeshedding, a couple of judgment calls). I also wasn't sure if/how to handle non-integer values produced by jr:count. So that isn't addressed at all yet. Do we want to address it here?

[changeset-bot]

🦋 Changeset detected

Latest commit: 46a3d14

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@getodk/xforms-engine	Minor
@getodk/web-forms	Minor
@getodk/scenario	Minor
@getodk/ui-solid	Minor
@getodk/common	Minor
@getodk/xpath	Patch

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]

I also want to call out that very little in this change is really new! The vast majority of the implementation change is moving stuff into a base class shared by both repeat range types. I did briefly consider skipping that and implementing the count aspects in a constructor branch, but I think it’s helpful to see where they split more clearly. And to hew more closely to their slight differences in client API capabilities.

[eyelidlessness]

I just remembered to mention another bit of functionality that we might want, but isn't handled here: preserving and restoring previous added repeat instances. I believe this is something @seadowg mentioned when we spoke about it a while back. It is certainly possible, likely even trivial, but I didn't want to be too presumptuous about it without some opportunity for discussion.

[seadowg]

I just remembered to mention another bit of functionality that we might want, but isn't handled here: preserving and restoring previous added repeat instances. I believe this is something @seadowg mentioned when we spoke about it a while back.

I don't recall this sadly!

[sadiqkhoja]

This looks great 🎉

Can we add couple of tests where:

• jr:count is static number, existing test suite static values is setting static values to input /data/rep-count
• it shows that "jr:count is intended to be the ultimate determiner of the number of repeat instances so users should not be able to add or remove instances."

[sadiqkhoja]

It can be child of RepeatRangeControlledNode as well, right?

[sadiqkhoja]

I agree with this.

[eyelidlessness]

I think we can consider producing a warning about this (and things like it) in the future.

[eyelidlessness]

• jr:count is static number, existing test suite static values is setting static values to input /data/rep-count

Added in 8c6b5aa.

• it shows that "jr:count is intended to be the ultimate determiner of the number of repeat instances so users should not be able to add or remove instances."

Does this test cover what you're looking for? If not, can you say more about what you'd like to see?

---

I also want to make sure to follow up on our side discussion about naming. Now that we've both had some time to let it marinate, which of these feels right to you?

• RepeatRangeControlled[Node]/RepeatRangeUncontrolled[Node]
• RepeatRangeManaged[Node]/RepeatRangeUnmanaged[Node]
• Something else?

[sadiqkhoja]

LGTM 🎉