fix(#687): calculate jr:count before attached#688
Conversation
🦋 Changeset detectedLatest commit: 5d95c0e 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 |
| } catch { | ||
| // likely because it's not yet attached - try again later | ||
| return defaultValue; | ||
| } |
There was a problem hiding this comment.
This is the change...
When jr:count was a constant then line 120 was running and returning the correct value. However when it was not a constant (eg: reading the value from a node) then it was firing twice, first before the root was attached in which case it returned the default value of 0, and secondly after it was attached where it returned the actual value from the node. Because the non-zero value was returned after the root was attached, it triggered the new repeat event which it should not do, because that will overwrite a value set by the user.
This solution is to evaluate the expression even if the root is not yet attached so the child repeats are created before attachment and therefore don't trigger the event.
This caused a separate problem because some tests failed because the expression may fail if the node doesn't exist yet. I came up with two ways to solve this...
- Change the xpath function implementations (mostly
distanceandchoice-name) to gracefully handle expressions for nodes that don't exist but returning null values. This feels like changing a lot just to make the engine's order of operations happy. - Just catch the exception and return the default, which is the value that was being returned in the old code anyway. This should work because when the nodes are attached the reactive code will fire again and this time it'll go to line 129 and either pass or fail as expected.
I'm not 100% comfortable with this approach. It does feel more correct to do as much evaluation as early as possible, but it would be nicer to have a different definition of "attached" that marks the end of model binding but before users can interact with the form. But, I don't know what that would be...
There was a problem hiding this comment.
I agree that changing the XPath functions is too invasive. The try/catch approach is a good pragmatic choice. It safely handles the jr:count while keeping the existing fallback for everything else.
| } catch { | ||
| // likely because it's not yet attached - try again later | ||
| return defaultValue; | ||
| } |
There was a problem hiding this comment.
I agree that changing the XPath functions is too invasive. The try/catch approach is a good pragmatic choice. It safely handles the jr:count while keeping the existing fallback for everything else.
2 participants
Closes #687
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
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.
What's changed