-
Notifications
You must be signed in to change notification settings - Fork 191
feat: Add two-way computed signals via WritableSignal.map(getter, set… #23370
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
Conversation
…ter)
This enables mapping a single field from a record or bean to a separate
writable signal that supports two-way bindings. The mapped signal
propagates changes back to the parent signal.
Added:
- SignalSetter<P,C> interface for immutable update patterns
- SignalModifier<P,C> interface for mutable in-place modification
- MappedWritableSignal class using parent.update() for immutable values
- MappedModifySignal class using parent.modify() for mutable values
- WritableSignal.map(getter, setter) default method
- ValueSignal.map(getter, modifier) overload for mutable patterns
Example usage with records:
WritableSignal<Todo> todoSignal = new ValueSignal<>(new Todo("Task", false));
WritableSignal<Boolean> doneSignal = todoSignal.map(Todo::done, Todo::withDone);
checkbox.bindValue(doneSignal); // Two-way binding to done property
Example usage with mutable beans:
ValueSignal<Todo> todoSignal = new ValueSignal<>(new Todo());
WritableSignal<Boolean> doneSignal = todoSignal.map(Todo::isDone, Todo::setDone);
checkbox.bindValue(doneSignal); // Two-way binding using in-place modification
Slack thread: https://vaadin.slack.com/archives/C6RAXJATF/p1769700370488139?thread_ts=1769696326.293389&cid=C6RAXJATF
https://claude.ai/code/session_01TxPx9BEUzohqox27CLezT5
Wrap lines exceeding 80 characters to comply with Vaadin code style. https://claude.ai/code/session_01TxPx9BEUzohqox27CLezT5
The original implementation of two-way computed signals introduced an ambiguous overload: WritableSignal.map(getter, setter) and ValueSignal.map(getter, modifier) had the same arity but different functional interface parameter types. Java's type inference could not always disambiguate between SignalSetter (returns parent type) and SignalModifier (returns void) when using method references or lambdas. Renaming ValueSignal's method from map to mapMutable resolves this ambiguity and makes the distinction clear in the API: map() creates immutable value mappings, mapMutable() creates in-place modification mappings. Fixes #23370 https://claude.ai/code/session_019LonZZA33kKDmk7jCgN7cT
| * @see WritableSignal#map(SignalMapper, SignalSetter) | ||
| */ | ||
| @FunctionalInterface | ||
| public interface SignalSetter<P, C> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure about this name but I don't have any good counter proposal right now. The problem is that the callback doesn't itself "set" the value but instead creates a value that the caller will use for setting. So from that point of view, this type is more of a "converter", "creator", or "reducer".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point from @Legioth. The name SignalSetter is misleading since the function doesn't set anything - it creates a new parent value given the old parent and new child value.
Some naming alternatives to consider:
| Name | Rationale |
|---|---|
SignalReducer |
Follows the Redux pattern: (state, value) -> newState |
SignalValueReducer |
More explicit variant |
SignalCombiner |
Combines parent + child into new parent |
SignalWithMapper |
Reflects the withX() method pattern commonly used |
SignalValueCreator |
Creates a new value from parent + child |
I think SignalReducer fits well because the signature (P parent, C child) -> P is essentially a reducer function - it takes the current state and a value, and produces a new state. This is a familiar pattern from functional programming.
Would you like me to rename SignalSetter to one of these alternatives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Signal" prefix might be redundant in this context. I'd suggest ValueMerger since it creates a new outer value by merging the new inner value with the old outer value.
Legioth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at the tests yet
|
|
||
| @Override | ||
| public SignalOperation<C> value(C newChildValue) { | ||
| AtomicReference<C> oldChildValue = new AtomicReference<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than all the complexity in each method for capturing references to some old value from inside some callback and manually resolving operations, how about just adding support for a map operation on SignalOperation instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added SignalOperation.map() method (SignalOperation.java:119-131):
Transforms the result value using a mapper function
Preserves error propagation
Added CancelableOperation.map() override (CancelableOperation.java:68-92):
Returns a CancelableOperation (not just SignalOperation)
Cancelling the mapped operation also cancels the parent
isCancelled() delegates to parent
Simplified MappedWritableSignal (MappedWritableSignal.java:77-121):
Before (complex with AtomicReference and helpers):
public SignalOperation<C> value(C newChildValue) {
AtomicReference<C> oldChildValue = new AtomicReference<>();
CancelableOperation<P> parentOp = parent.update(parentValue -> {
oldChildValue.set(getter.map(parentValue));
return setter.set(parentValue, newChildValue);
});
return mapOperation(parentOp, oldChildValue);
}
After (clean and simple):
public SignalOperation<C> value(C newChildValue) {
return parent.update(
parentValue -> setter.set(parentValue, newChildValue))
.map(oldParent -> getter.map(oldParent));
}
The replace method still uses AtomicReference because it has conditional success/failure logic that can't be expressed through just map().
| AtomicReference<SignalOperation.ResultOrError<Void>> resultRef = | ||
| new AtomicReference<>(); | ||
|
|
||
| CancelableOperation<P> parentOp = parent.update(parentValue -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this could be much simpler like this:
var originalParentValue = parent.peek();
var oldChildValue = getter.map(originalParentValue);
if (!Object.equals(oldChildValue, newValue)) {
return new SignalOperation<>(new Error<>("Unexpected value"));
}
return parent.replace(originalParentValue, setter.set(originalParentValue, newValue));
There might be some ABA cases that are rejected in this way even though they could in theory be fine but I don't think that's an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| * License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| package com.vaadin.signals.local; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be in an impl package since the method creating these instances is defined to return WritableSignal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Add a map() method to SignalOperation and CancelableOperation that transforms the result value using a mapper function. This allows cleaner composition of operations. Use this new method to simplify MappedWritableSignal's value() and update() implementations, removing the need for manual AtomicReference tracking and custom operation mapping helpers. https://claude.ai/code/session_019LonZZA33kKDmk7jCgN7cT
…nt.replace Use a simpler approach that peeks the current parent value and delegates to parent.replace() instead of using update() with AtomicReference tracking. https://claude.ai/code/session_019LonZZA33kKDmk7jCgN7cT
Move MappedModifySignal from local to impl package since the mapMutable() method that creates these instances returns WritableSignal, making the concrete class an implementation detail. https://claude.ai/code/session_019LonZZA33kKDmk7jCgN7cT
The original implementation of two-way computed signals introduced an ambiguous overload: WritableSignal.map(getter, setter) and ValueSignal.map(getter, modifier) had the same arity but different functional interface parameter types. Java's type inference could not always disambiguate between SignalSetter (returns parent type) and SignalModifier (returns void) when using method references or lambdas. Renaming ValueSignal's method from map to mapMutable resolves this ambiguity and makes the distinction clear in the API: map() creates immutable value mappings, mapMutable() creates in-place modification mappings. Fixes #23370 https://claude.ai/code/session_019LonZZA33kKDmk7jCgN7cT
Update @see reference and example code in SignalModifier to use the renamed mapMutable method instead of the old map method name. https://claude.ai/code/session_019LonZZA33kKDmk7jCgN7cT
1c6cf78 to
5c7df2c
Compare
| @Override | ||
| public SignalOperation<C> value(C newChildValue) { | ||
| return parent | ||
| .update(parentValue -> setter.set(parentValue, newChildValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already discussed this offline but the outcome is non-trivial so it might be good to keep in a comment for future reference.
If we apply the change to the new item, then the user gets the impression that they happened to click immediately after it was changed and then they immediately realize that they can easily undo that accidental change directly from the same UI without having to find the old item in the UI to see if their change ended up there
| return parent.isCancelled(); | ||
| } | ||
| }; | ||
| result().thenAccept(resultOrError -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated code in SignalOperation. Could be extracted to a protected helper method.
| } | ||
| } | ||
|
|
||
| record Pair<A, B>(A first, B second) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confusing to have two different test types that have essentially the same structure. Could all tests use the same test type instead?
| } | ||
|
|
||
| @Test | ||
| void map_withNullParentValue_handlesGracefully() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pointless test?
| } | ||
|
|
||
| @Test | ||
| void map_stringField_works() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointless test
| } | ||
| } | ||
|
|
||
| static class MutablePair<A, B> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant and confusing with two separate test classes?
| assertFalse(doneSignal.value()); | ||
|
|
||
| todo.setDone(true); | ||
| todoSignal.modify(t -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test makes more sense if the modification is done inside modify instead of before it since that's the recommended way of doing a mutation
| } | ||
|
|
||
| @Test | ||
| void map_stringField_works() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems redundant
| * @see WritableSignal#map(SignalMapper, SignalSetter) | ||
| */ | ||
| @FunctionalInterface | ||
| public interface SignalSetter<P, C> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Signal" prefix might be redundant in this context. I'd suggest ValueMerger since it creates a new outer value by merging the new inner value with the old outer value.
Extract duplicated result forwarding logic from SignalOperation.map() and CancelableOperation.map() into a protected forwardMappedResult() helper method. Add comment explaining why update() is used in MappedWritableSignal.value() for better concurrent modification behavior.
Rename SignalSetter to ValueMerger with merge() method to better describe its purpose of merging inner values into outer values. Consolidate test types by using single Todo/MutableTodo classes with priority field instead of separate Pair types. Remove redundant tests and fix modify() usage in MappedModifySignalTest.
signals/src/test/java/com/vaadin/signals/impl/MappedWritableSignalTest.java
Outdated
Show resolved
Hide resolved
Use boolean inversion for update() tests instead of adding an int field. Keep Todo/MutableTodo simple with just text and done fields.
|



…ter)
This enables mapping a single field from a record or bean to a separate writable signal that supports two-way bindings. The mapped signal propagates changes back to the parent signal.
Added:
Example usage with records:
WritableSignal todoSignal = new ValueSignal<>(new Todo("Task", false)); WritableSignal doneSignal = todoSignal.map(Todo::done, Todo::withDone); checkbox.bindValue(doneSignal); // Two-way binding to done property
Example usage with mutable beans:
ValueSignal todoSignal = new ValueSignal<>(new Todo()); WritableSignal doneSignal = todoSignal.map(Todo::isDone, Todo::setDone); checkbox.bindValue(doneSignal); // Two-way binding using in-place modification
Slack thread: https://vaadin.slack.com/archives/C6RAXJATF/p1769700370488139?thread_ts=1769696326.293389&cid=C6RAXJATF
https://claude.ai/code/session_01TxPx9BEUzohqox27CLezT5
Description
Please list all relevant dependencies in this section and provide summary of the change, motivation and context.
Fixes # (issue)
Type of change
Checklist
Additional for
Featuretype of change