feat: enhance Binder with cross-field validation #23360
Conversation
Adds Binder.Binding.value() to be used in field binding validators to read value of another field. The Binder automatically runs validators inside a ComponentEffect#effect context, making validators and converters reactive to signal changes. Part of #23298
tltv
force-pushed
the
feat/binder-signal-validation
branch
from
a0fa6ef to
2569dd3
Compare
Bean level validators throws Binder.InvalidSignalUsageError error when Signal.value() is called in them. Fixes: #23364
|
|
||
| private transient Registration signalRegistration; | ||
|
|
||
| private boolean initialSignalRegistration = true; |
There was a problem hiding this comment.
Seems to be well aligned with the existing valueInit flag. Would it make sense to reuse that one?
| if (signalRegistration == null | ||
| && getField() instanceof Component component) { | ||
| initialSignalRegistration = true; | ||
| signalRegistration = ComponentEffect.effect(component, () -> { |
There was a problem hiding this comment.
As I understand, this makes validators and converters signal-reactive for validation purposes. Given that I use a signal in my converter function, what happens regarding value updates arising from a signal-triggered change?
After reading the code, seems that such updates won't make their way to fields. If so, is it intentional? Would it make sense to run converters outside of effect context?
There was a problem hiding this comment.
It's intentionally running also converters inside the effect. But in practice I don't believe signals are actually used in converters. If they are, then validation reruns also on those signal value changes.
I didn't see earlier nice technical solution to leave converters out of scope here. But now I see one, it's to wrap each converter execution inside UsageTracker.untracked(...). Do you think this would make sense?
| * "Both fields must match") | ||
| * .bind("password"); | ||
| * | ||
| * UI.getCurrent().add(passwordField, confirmField); |
There was a problem hiding this comment.
I'd avoid suggesting UI.getCurrent() in the javadoc, would it be enough to just keep add(passwordField, confirmField); ?
| if (signalRegistration != null) { | ||
| signalRegistration.remove(); | ||
| signalRegistration = null; | ||
| internalValidationTriggerSignal = null; |
There was a problem hiding this comment.
Does this need initialSignalRegistration=true as well?
| * // error | ||
| * | ||
| * // or same with a Signal: | ||
| * confirmSignal.value("secret"); // passwordField shows validation |
There was a problem hiding this comment.
Javadoc could also show the initialization of confirmSignal and how it is bound to the confirm password field.
There was a problem hiding this comment.
Actually confirmSignal is not used in the validator at all.
Perhaps it's better to add a validator to the confirm password field instead of password field.
All in all, I'd suggest to document exactly the codes from the ticket. It uses both types of dependency - binding value and signal value.
| /** | ||
| * Error thrown when a signal is used incorrectly. | ||
| */ | ||
| public static class InvalidSignalUsageError extends Error { |
There was a problem hiding this comment.
I'd suggest extending IllegalStateException as Error is something that indicates serious problems.
| throw new InvalidSignalUsageError( | ||
| "Detected Signal.value() call inside a bean level validator. " | ||
| + "This is not supported since bean level validators " | ||
| + "are not run inside a reactive effect."); |
There was a problem hiding this comment.
I'd add a sentence "Using Signal.value() is only supported in field level validators."
mshabarov
left a comment
mshabarov
left a comment
There was a problem hiding this comment.
Few minor comments, but looks good to me and also works as expected with an example from the ticket.
* feat: add binder validation status signal Adds Binder.getValidationStatus() method to get read-only signal for BinderValidationStatus. Fixes: #23300 * fix single binding validation status change * fix unresolved validation status change
|
Adds
Binder.Binding.value()to be used in field binding validators to read value of another field. The Binder automatically runs validators inside aComponentEffect#effectcontext, making validators and converters reactive to signal changes.Fixes: #23298