Engine support for constraint, required validation
#154
Conversation
🦋 Changeset detectedLatest commit: 200c222 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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/constraint-required-validation
branch
from
8ef324a
to
75675dc
Compare
eyelidlessness
force-pushed
the
features/constraint-required-validation
branch
from
01afabc
to
4792e36
Compare
There was a problem hiding this comment.
This is super nice 🎉
Changes related to text and parsing make sense to me, it is beautiful.
I have dropped few minor questions and comments inline. In the meantime, I am going to try couple of quick experiments in UI, to see how it functions. Excited to get this in!
packages/xforms-engine/src/lib/reactivity/createComputedExpression.ts
Outdated
Show resolved
Hide resolved
packages/xforms-engine/src/lib/reactivity/validation/createValidation.ts
Show resolved
Hide resolved
packages/xforms-engine/src/lib/reactivity/validation/createAggregatedViolations.ts
Outdated
Show resolved
Hide resolved
|
Works great in |
eyelidlessness
force-pushed
the
features/constraint-required-validation
branch
from
b8b6197
to
3fb493e
Compare
|
I've added a quick and dirty fix for client reactivity of aggregated/parent node validity state. I confirmed directly in |
eyelidlessness
force-pushed
the
features/constraint-required-validation
branch
from
3fb493e
to
6ab2ce2
Compare
|
@lognaturel it seems like a safe assumption that validation should not be applied to non-relevant nodes (f2cbd02, test added in b14db09). But I've been surprised by some aspects of JavaRosa non-relevant node behavior before! Feel free to correct me if there's something off about this assumption. |
Yes, that seems right to me! Validation status can't be used in dependent calculations so I don't see any benefit to computing it. It's also very important that validation isn't applied to non-relevant nodes because that could block submission. |
eyelidlessness
force-pushed
the
features/constraint-required-validation
branch
from
f2cbd02
to
1b06b2c
Compare
As discussed in #140 Based on 4c38a22, with some refinements: - Each condition’s validity state is now represented by `valid: boolean` - Minor naming adjustments - Nailed down an initial pair of message constants for engine-fallback messages - Added a method to check whether a message is an engine-fallback, in a way that allows type narrowing and produces those message constants as types - Added a ton of JSDoc documentation clarifying the design, expected usage, some anticipated performance caveats, etc
Note: as described in the included TODO.md, this begins moving some parsing logic into a dedicated directory. This is a direction I’d wanted to take for some time. It felt particularly appropriate now because parsing these aspects of text no longer fits in the current body directory (itself entirely concerned with parsing, but only implicitly so), and it felt particuarly awkward to introduce another parsing-related top-level source directory (moving further *away* from this intent rather than inching towards it). I’d like to consider moving the rest of parsing logic into this directory within the same PR. It will almost certainly be just moving files and updating imports from here (and then TODO.md will never need to appear).
Note: this is intentionally a naive first pass! It does not defer computation as extensively as it could. I believe it is worth having that possibility documented, but measuring real world performance before optimizing further.
Note: this is also a naive first pass! Aggregated results are not stored in a client state object, but rather computed up from leaf nodes (where both engine/client state are read to ensure reactivity on both sides of that boundary).
…ngle node node-set This affects some `scenario` validation tests. It will also fix a few other tests which currently fail because of this difference in XPath/XForms boolean semantics (details in JSDoc).
- Form-specified constraintMsg, requiredMsg - Default/fallback messages for both conditions, as provided by the engine
In theory, we could do something like this to simplify children state logic as well, at least until we have a better general solution to storing/serializing/materializing complex nested state like direct references to node objects. I’d prefer to keep it isolated for now and think about priority of a more holistic solution.
As we discussed, this can be handled in a separate scope of work. The original commits are left intact for reference, we can rebase before merge if preferred.
Also eliminates most of the basis for the hacky custom client state solution, and so falls back to `createSharedNodeState`. This still isn’t ideal, as it’s redundantly storing a bunch of state that’s fundamentally derived. Worth considering whether the client interface should also accept an optional derived state factory (e.g. Vue’s `computed`, Solid’s `createMemo`).
…node and form level)
Note: no change to aggregation necessary! Derived state is awesome.
eyelidlessness
force-pushed
the
features/constraint-required-validation
branch
from
1b06b2c
to
200c222
Compare
Adds support for validation of nodes with
constraintandrequiredbind expressions. Closes #140, supports #130.Message support, text parsing and state refactors
A big chunk of this change involves a refactor of the existing parse structures for form-defined text (
<label>and<hint>), to support parsing very similar structures for validation messages (jr:constraintMsg,jr:requiredMsg). That refactor makes these additional judgement calls:Establishing a new top-level
parsedirectory in the engine source. I believe it would be good, either in this PR or in a followup, to move the remainder of parsing logic into this directory for better clarity about where parsing logic ends and runtime logic begins. Doing so for this portion of parsing felt most sensible now because the logic was previously body-specific, and its newfound generality between model and body would have placed it somewhere new regardless.Adapting the notion of
messageSource(from Engine/client API design:constraintandrequiredvalidation #140) to a more general concept oforiginon all parsed text range structures. As in that design proposal, validation messages will have anoriginof either "form" or "engine". There was also already a case where the engine produces text ranges that are not strictly defined by the form (<item>and<itemset>labels, which are derived from their respective<value>states when not accompanied by a<label>). This existing case is treated as a thirdorigin, "form-derived".Introducing a more explicit distinction between "translation" and "reference" text chunks. This felt like it would add some clarity where validation messages do not have a concept equivalent to the
<label ref>aspects of spec (which is overloaded to support bothjr:itextcalls as well as subtree references within<itemset>).The runtime state aspects of text have also been refactored a bit, both to support the above changes where necessary, and to simplify/clarify:
originselect item labelsKeeping state simple for now
Leaf/value nodes
While the design and types suggest a potentially lazier computation of validation state, this PR defers the implementation aspect of that. In this PR, leaf/value node validation state is computed and propagated just like any other shared node state. This will have all of the same efficiencies of fine-grained reactivity as any other aspect of the engine internals utilizing Solid reactivity.
I believe this PR is large enough, and there's enough complexity in the runtime aspect, that there's good reason to defer further optimization on those bases. I also think this will be a good opportunity to get a feel for the performance impact on real forms, and make an informed decision on whether to measure and optimize further from there.
Ancestor nodes
Aggregated validation state (i.e. on ancestor nodes) is implemented internally as derived/computed state. It is memoized at each level of hierarchy. An earlier pass involved a fairly hacky mechanism to make this client-reactive, but simplifying the aggregated error interface (removing the
nodeobject property) eliminated most of the basis for this. The derived state is now client-reactive with the samecreateSharedNodeStateabstraction we use for buildingAnyNode.currentState. Note that this is still non-ideal, as it will redundantly create the same state referencing a single leaf node at each level of hierarchy all the way up to the root.Testing
Another big chunk of this change involves various updates to
scenariotesting (and related support logic) to show the impact of the change on existing tests ported from JavaRosa in #110. For the most part, all of the tests that would be expected to pass now do! As far as I'm aware, any remaining failures exercising validation are failing for unrelated reasons (e.g. testing [de]serialization).Another few tests have been added specifically to test the messaging aspects of validation, as that was a gap in existing
scenariotests.An additional test has also been added, demonstrating that validation is only effective on relevant nodes, both at the question/leaf node level, and at the form/ancestor aggregated level. (A subsequent fix has also been added, as I'd forgotten to address this in the first pass!)