Several repeat-related fixes #135
Merged
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
🦋 Changeset detectedLatest commit: 46bf4ec The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
This was referenced Jun 26, 2024
sadiqkhoja
approved these changes
Jun 27, 2024
| @@ -272,7 +272,7 @@ describe('Tests ported from JavaRosa - repeats', () => { | |||
| * expandReference call in Triggerable.apply which ensures the result is | |||
| * updated for every repeat instance. | |||
| */ | |||
| it.fails('updates repeat count, inside and outside repeat', async () => { | |||
| it('updates repeat count, inside and outside repeat', async () => { | |||
There was a problem hiding this comment.
Porting notes need to be updated since the test is no longer failing.
… when combined with `createMemo` and children state access.
… by extending the lookup of subscribable nodes to produce all nodes for a dependency’s node-set. Note that this is a correctness improvement, but the solution is naive and likely comes with performance costs: - Some expressions do not need to subscribe to all of the matching nodes, and may execute more often than needed as a consequence - Lookup now always begins at the instance root, where it previously walked relative to the context node - While this doesn’t do full tree traversal, it does traverse more of a given match’s siblings, and may traverse parallel subtrees where it hadn’t previously We should consider measuring the performance impact of this change on real forms so we can determine whether it’s a meaningful optimization target. If it is, we’ll likely be looking at either: - static analysis of XPath expressions and producing different dependency resolution strategies depending on the expression - the old favorite, decoupling XPath evaluation from the browser/XML DOM
There is no logic which would have intentionally made these cases pass, and there are others like them failing already. This does not represent a meaningful regression; if anything it makes the same class of tests easier to address together. This also changes those error-checking form-init-error smoke tests to not produce a `Scenario` instance in the init error assertions. This avoids an issue where assertion failure would produce enormous shell/console output where the `Scenario` instance clearly is not an error/Promise rejection.
This doesn’t fix a bug in and of itself, but it warrants a separate commit because it touches a fair bit of code. It eliminates some redundancy, introduces a bit of complexity to do that, but mostly it enables downstream work on bugs with these overlapping concerns: - Earlier computation of `relevant`, allowing us to correctly write blank values where a node has a default value but is initially non-relevant. It’s expected that will **also support** later feature work, including: - `setvalue`/`odk-instance-first-load` - editing submissions (hunch; TBD pending clarification of requirements) - Earlier computation of `readonly`, mainly for consistency with `relevant` as both have inheritance logic - Correcting inheritance logic around both `relevant` and `readonly` as applied to repeats.
… extensive clarification in PORTING NOTES comments. This is additional set up to demonstrate two incoming bug fixes: - Clearing default values of nodes which are non-relevant on form load/init - Correctly inheriting non-relevance of/within repeat ranges
The nature of this fix is that `relevant` computation is initialized and performed **before** `value` state is initialized. This is necessary because: - in the original initialization order, the `isRelevant` check references different reactive dependencies before and after initialization of `value` - before `value` state init: the dependency would return a default value as the real `relevant` computation was not yet set up - after `value` state init: the dependency would not trigger a `value` update clearing the non-relevant value, because `value` had not yet subscribed to the newly initialized `isRelevant` state dependency This **also** moves all other `DescendantNode` shared state initialization to the construction phase. In turn, it also allows all `DescendantNode` subclasses to reference those initialized states directly in creation of their shared node state/client-facing `currentState`. In the course of this, it also felt easier to reason about the various internal-facing reactive accessors **as accessors**, i.e. as zero-arity function calls rather than as getters. - - - Also notable: this eliminates the awkward `getInitializedState` workaround for `relevant` and other aspects of `DescendantNode` state init. Pure speculation as I write this, but it seems possible that I’ll be able to eliminate further init awkwardness like this (including possibly restoring synchronous `Root` construction).
As suspected in previous commit, none of this async init awkwardness is necessary now. The `initializeForm` interface still has to be async because it handles resource fetching. We can consider breaking up those aspects of init in the future if that seems like it would be beneficial from a client perspective.
Moving `relevant` (etc) state init earlier in node construction also fixed this timing bug!
Extensive commentary in JSDoc, some of which is probably worth discussing in review.
eyelidlessness
force-pushed
the
bugs/repeats-grab-bag
branch
from
ae84d4e
to
46bf4ec
Compare
eyelidlessness
added a commit
that referenced
this pull request
Jul 19, 2024
As discussed in review, it’s highly unlikely this is actually needed anymore! Brief backstory context in case we ever have reason to doubt this: 0. The WHO VA form, which I’ve used as a frame of reference for many things, uses two references to `null` that don’t resolve to anything. The inferred intent is to treat `null` as a keyword, just as it would be in languages with the Billion Dollar Mistake. 1. In early prototyping (pre Web Forms project), the WHO VA form’s references to `null` produced errors where there was assertion logic checking that expected dependencies were found. 2. Early Web Forms work produced the same, for a time at least producing errors in the console. This produced a lot of distracting noise that made it harder to identify actual implementation errors in dependency resolution. 3. Subsequent improvements to dependency resolution have been successful enough that we’ve even eschewed logging when a dependency lookup doesn’t resolve (though I sometimes add it in local dev for testing/validation), at least as of #67. 4. As of #135, dependency resolution was expanded to be form-wide and match all possible nodes; there’s no case where a `null` reference **should** match a node and won’t. I can imagine these potentially useful followup changes: 1. On parse, identify any path sub-expressions which **will never** resolve to any node in the form. 2. Short circuit lookup of such never-resolvable paths. There’s no sense walking the full form tree for something we know we won’t find! 3. Potentially warn when such never-resolvable paths are found. This wouldn’t be particularly useful for the `null` NameTest specifically (where the intent at least in WHO VA is clearly a null reference), but it could be useful for catching typos and other mistaken references like hierarchy confusion.
eyelidlessness
added a commit
that referenced
this pull request
Jul 22, 2024
As discussed in review, it’s highly unlikely this is actually needed anymore! Brief backstory context in case we ever have reason to doubt this: 0. The WHO VA form, which I’ve used as a frame of reference for many things, uses two references to `null` that don’t resolve to anything. The inferred intent is to treat `null` as a keyword, just as it would be in languages with the Billion Dollar Mistake. 1. In early prototyping (pre Web Forms project), the WHO VA form’s references to `null` produced errors where there was assertion logic checking that expected dependencies were found. 2. Early Web Forms work produced the same, for a time at least producing errors in the console. This produced a lot of distracting noise that made it harder to identify actual implementation errors in dependency resolution. 3. Subsequent improvements to dependency resolution have been successful enough that we’ve even eschewed logging when a dependency lookup doesn’t resolve (though I sometimes add it in local dev for testing/validation), at least as of #67. 4. As of #135, dependency resolution was expanded to be form-wide and match all possible nodes; there’s no case where a `null` reference **should** match a node and won’t. I can imagine these potentially useful followup changes: 1. On parse, identify any path sub-expressions which **will never** resolve to any node in the form. 2. Short circuit lookup of such never-resolvable paths. There’s no sense walking the full form tree for something we know we won’t find! 3. Potentially warn when such never-resolvable paths are found. This wouldn’t be particularly useful for the `null` NameTest specifically (where the intent at least in WHO VA is clearly a null reference), but it could be useful for catching typos and other mistaken references like hierarchy confusion.
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.
This PR includes a handful of fixes and improvements addressing issues found in #110, generally with a common repeat-related theme.
The commit notes go into some detail of each fix. I'll highlight some high level details here as well.
ad607dc Fix
InconsistentChildrenStateErrorcases that currently affectscenario. This fix shouldn't affect theweb-formsVue UI client (as it only affects clients using Solid), but it was important to fix first so that the other fixes won't be obscured by introducing this same error.0212b9f Fix a variety of issues where behavior is correct for the first repeat instance in a range (or within that repeat instance, or referencing into it) but incorrect for subsequent repeat instances. The change doesn't address all such cases, but it is probably further reaching in real usage than the set of newly passing tests suggests. I expect that it will address quite a few cases where repeat functionality feels broken/incomplete.
Note that this is followed by another commit (57883df) marking several form init smoke tests failing that did not previously. This may seem like a regression, but I don't think it really is. Those tests had previously satisfied a broad check for producing an error, and their previous passing status was essentially a coincidence. We'll likely address these more meaningfully in some of the work related to Handle errors and warnings in a consistent way #80.
19016e6 (and eea137d) Fix a few issues related to the order of reactive state initialization for each form node. The mechanism of the fix involves moving the initialization of
reference(i.e. the effective XPath reference to the node, which may change for repeat instances and their descendants) as well as shared bind computations—relevant,readonly,required—earlier in node initialization... effectively before any other reactivity for all non-root nodes.The immediate benefit, as visible in the main commit for the fix, is that we now correctly clear default values of non-relevant nodes on form load. While that isn't specifically repeat-related, the other benefit of this change is that it eliminates the aspect of form/repeat instance init where certain aspects of reactivity are not initially "ready" when they need to be initially referenced. As such, and reflected in followup commits, this fix also resolves any remaining asynchrony1 during form init (8642ed7). For repeats, this also fixes edge cases where certain aspects of reactive state lag after creating now repeat instances (90e1ad6).
This broadly reaffirms our current intent that form state is reconciled fully, synchronously, for every form action (until a need to deviate arises).
b44beec Fix some cases where
relevantstate was incorrectly inherited when applied to repeat instances themselves. The nature of this issue was thatRepeatRangenodes (which are a fiction for our runtime data model, not real nodes in the primary instance) were incorrectly evaluatingrelevantexpressions intended for theirRepeatInstancechildren.The first (correctness) problem with this was that the expression would be evaluated against the wrong context, as
RepeatRange.contextNodereferences the repeat range's parent element in the primary instance tree. The second problem: if this expression evaluated tofalsefor aRepeatRange, that would be inherited from all of itsRepeatInstancechildren even if the expression properly evaluatestruefor the instance itself. These issues are resolved.There are also
scenariotests which consider the relevance of the repeat range itself... or at least, they check the presence of a prompt to create new instances of the repeat range, and we've ported those tests to check for that range's relevance. We now determine thisRepeatRangerelevance state by a heuristic, where the range itself is relevant if either of these cases are true:RepeatRangehave >=1 relevantRepeatInstancechildren?RepeatRangehave 0RepeatInstancechildren at all?It's entirely possible this is the wrong heuristic. It's also entirely possible that there is a more appropriate way to represent the presence or absence of a repeat "prompt" besides relevance state. I am certain that
scenariowill need different logic when we introduce support forjr:count(Implement jr:count #87).Footnotes
Besides resource fetching, which is inherently async and which is at least presently handled before the now-synchronous
Rootinitialization. ↩