Skip to content
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

Fix: client reactivity of translated select item labels #114

Merged
merged 5 commits into from
May 15, 2024

Conversation

eyelidlessness
Copy link
Member

Fixes #83.

Supersedes and closes #102 as well. Differences from that PR:

  • Breaks the fixes for these (related, but) distinct bugs into separate commits:

    • Translated dynamic select itemset labels (e.g. <select1><itemset nodeset="instance('choices')/root/item"><label ref="jr:itext(itextId)"/>…) are not client-reactive.

    • Translated labels of static select items (e.g. <select><item><label ref="jr:itext('foo')"/>…) are also not client-reactive.

    The distinction is discussed in Apparently selectNode.currentState.valueOptions are not “reactive” to language change #83. I expect having separate commits could be helpful in the future, whenever we need to reason about the slight differences in how each case flows through the engine's reactive internals.

  • Adds tests for both bugs, exercising reactivity of these scenarios:

    • Trigger an effect on load, reflecting initial state. This behavior would vary depending on the client's reactive factory and scheduling particulars; we want to be sure it's possible to achieve (even if a client may opt not to exercise it), so it's a distinct test case.

    • Trigger an effect on each change to the form's active language, for each translated label, reflecting the appropriate translation after each update.

    • Do not trigger an effect on repeated reassignment of the currently active language. This is/should be a reactive no-op, both internally and for clients. There's nothing special happening in this PR to achieve that, it relies on the behavior implemented in createSharedNodeState. While there are more general ways we could (and maybe should) test that generalization, it felt appropriate to test here because unnecessary translation reactivity is an area that could easily slow down client performance across a wide variety of larger forms.

  • While the fixes are conceptually quite similar, I think there's some improvement to clarity in where/how some internal-reactive accessors are called. In particular I think it's important to preserve the separation between setup of reactive accessors, and their subsequent/downstream calls.


Also in this PR, CI checks that were used to skip certain jobs based on a branch's changes are removed (at least temporarily). In the past we've tolerated CI running seemingly extraneous jobs. But in this case, it was observed that no jobs were run for any package downstream from the change. That's risky enough that I think it's appropriate to address in the same PR. If we think it's important to reintroduce such change-based filtering (which we may not!), I'd recommend we file a separate issue so we can discuss how to do it right.

Several jobs which would potentially be affected by the engine changes on this branch are not being run, because they don’t check for engine changes.

This may be temporary, but it’s the simplest/quickest naive solution.
Copy link
Contributor

@sadiqkhoja sadiqkhoja left a comment

Choose a reason for hiding this comment

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

selectField.scope.runTask at createSelectItems.ts:34 looks redundant to me, otherwise this looks good.

Tests capture the bug correctly, I ran those tests locally to ensure they fail without the fix

Per discussion of keeping/extending local reasoning about reactive scope.
@eyelidlessness eyelidlessness merged commit 24277c2 into main May 15, 2024
70 checks passed
@eyelidlessness eyelidlessness deleted the fixes/83-reactive-SelectItem-labels branch May 15, 2024 19:23
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.

Apparently selectNode.currentState.valueOptions are not “reactive” to language change
2 participants