Thoughts about conventions and naming for binding signals to components?

Flow
1

I’m currently planning the details for the next phase of introducing reactive UI state managment in Flow: adding signal-based APIs to all the components.

The basic use case is to directly hook up various parts of a component’s configuration to UI state that is defined as a signal. Two trivial usage examples along with how they can be implemented with what’s in Vaadin 24.8:

  • Set the heading text based on the name of the currently selected product.
H1 heading = new H1();
ComponentEffect.format(heading, H1::setText, "Editing: %s",
    selectedProduct.map(Product::getName));
  • Show one section of a form only if a checkbox is checked.
ValueSignal<Boolean> checked = new ValueSignal<>(false);
ComponentEffect.bind(section, checked, Component::setVisible);
// making the checkbox to update the signal is out of scope for this discussion

The one thing that is obvious is that there should be instance methods for those ComponenEffect helpers, e.g. section.someInstanceMethod(checked);. Components mainly centered around showing a text should also have shorthand constructors for using a signal value directly or 1…n signals with a format string: new H1("Editing: %s", selectedProduct.map(Product::getName));

Method semantics

The first open question, before we even get to the actual name of someInstanceMethod, is how this method should behave in relationship to the regular setter and multiple invocations of the method itself. Typical signal use would be to set up the binding exactly once and then not directly touch that aspect of the component instance after the initial setup. It’s typically a deveoper mistake if there’s also some line of code that does section.setVisible(true) or another invocation of section.someInstanceMethod(anySignal). There could also be an opposite perspective: if the visibility of that component is bound to the signal, then it would be convenient if section.setVisible(true); would also propagate the value to the signal.

There are thus three different options for how setVisible(true) should behave if an explicit signal binding is active:

  1. Update the value of the bound signal (assuming the signal is readable).
  2. Use the provided value and unregister the signal binding.
  3. Throw IllegalStateException. You must explicitly unregister the signal binding (using a Registration returned from someInstanceMethod before setting a static falue.

Similarly for running someInstanceMethod again with a different signal instance:

  1. Set up another signal binding. The component uses the value from the signal that has most recently updated. Potentially confusing in some cases and very powerful in other cases.
  2. Set up a new signal binding after unregistering the previous signal binding.
  3. Throw IllegalStateException just as for setVisible(true).

Method naming

The other big question mark is what pattern to use for naming methods like this.

  1. One obvious candidate would be to stick to the same name setVisible(true) and setVisible(someSignal). This might be problematic if the “right” answer from the previous questions would make the methods have significantly different semantics.
  2. Different semantics can be addressed with a different name, e.g. bindVisible(someSignal). This does instead have a negative impact on discoverablity since it might be that not all regular setters have corresonding “bind” methods (you would use those through ComponentEffect instead) and it would require an extra step to check if it’s available for your case.
  3. Show the semantic difference but use a familar prefix: setVisibleBinding(someSignal). The main drawback here is that the name is slightly more verbose.

What’s your take?

I’m leaning towards IllegalStateException in the two first cases just to help avoid mistakes even though I realize it might be annoying in some special cases. Would you agree?

I don’t have a strong sense in either direction for the naming pattern so that’s where I would be very happy to hear any insights from you.

2

Method semantics:

I would go with the setXyz(value) updates the bound signal, as this should help with refactoring existing code. But I could also agree on starting with throwing an IAE, for now. If too many cases come up, where it annoys people, it is easier to get rid of the exception and switch to a more convenient variant (at least I hope so).

Method naming:

Strongly against setXyz(signal). The problem with setXyz is the expectation behind that naming convention. Since it is strongly connected with the POJO world, where it simply set the content of a specific field in most cases (with maybe some additional checks etc.), people will most likely expect that behavior: writing the value of the signal to the underlying field. That the given signal will be synchronized with the referenced component property afterwards is not clear enough by the method name IMO.

Therefore +1 for the bindXyz variant.

Regarding the “it might not be there for every setXyz”. The same problem exists with the overloaded setXyz: here you have to check, too, if there is a setXyz(signal) variant or not. It will be a bit easier/faster, since the IDE will list it, but at least in Intellij I can type xyz and then get setXyz and bindXyz or just the first one.

Variant 3 sounds like good compromise, but for instance the given example setVisibleBinding is confusing in its name. What is a visible binding? :smile:

3

For one-way bindings (effects), the signal seems the logical owner to me: signal.effect(component::setX).

Two-way binding is not a “set”, I like option 2 for those: bindX. I see a positive impact on discoverability, except on day 1: after that, one gets used to find all supported signal bindings by just typing .b, without having to find them in the plethora of setX.

And I won’t propagate the real setX to the signal, if possible, as it is a component property. Use case: deliberate choice of showing a custom text in some cases, accepting that it can be overwritten. Like “no recent messages” if nothing from the signal after a timeout.

1 Like
4

Good point. It’s certainly easier to change in that direction though I’m sure some weirdo will get a regression because they do something important in a catch block. :wink:

That works if you’re actively looking for xyz but it’s more of a hazzle if you don’t know the exact name and just browse through the list of set methods.

This is an interesting twist that I didn’t think of. Would be convenient to significantly reduce the amount of new API we would have to introduce. The big drawback is that there’s no direct way of hooking up with the component’s attach and detach events which means that you would have to remember to manually close the registration to avoid leaking memory in case the signal instance lives longer than the component instance. We could make it work with the help of yet another ThreadLocal but I’m not sure we want to go that route.

5

You’re right: friendly syntax, but too hard to make it work correctly.

setText(signal) is fine for one-way binding, but it seems unnatural to me if it also work the other way. That really screams bindText(signal) to me. So, both are actually useful. Not sure if the other direction is needed: update the signal without reading it. bindText(signal, direction)?

6

Method semantics

I’d err on POLA here. Also are there legit use-cases known or think-able?

There are thus three different options for how setVisible(true) should behave if an explicit signal binding is active:

  1. Update the value of the bound signal (assuming the signal is readable).

This would change the value of a signal, that might be used on other places. So this would be a big surprise: I called the setter of some component (by accident) and at the other side of my application something switched. Also what to do, if the signal can not accept a value?

  1. Use the provided value and unregister the signal binding.

This could mean bindings silently falling off. Given that signals help decoupling, it’s easy to imagine, that the places where one would be accident call setX is not near to the code that set up the binding and thus easy to spot.

So this option might be OK, if there is a way to make it explicit. As in “set a fixed value and unbind any signals”; but then this should not be the default but rather be part of the “singal bound to component attribute” interface.

  1. Throw IllegalStateException. You must explicitly unregister the signal binding (using a Registration returned from someInstanceMethod before setting a static falue.

Runtime exceptions are not great, but at least I get told, what went wrong, and it’s likely they get found in testing.

That said, there should be ways to “unbind” and ask the component if an attribute has a/many(?, see below) signals bound. Which makes me wonder, how much overhead or how much disruption this will add to the existing components. I guess some of it could be buried in the properties/attributes of the element and only the most important things get lifted into the interface of the component (e.g. “bind” is lifted, everything else resides in something nested).

  1. Set up another signal binding. The component uses the value from the signal that has most recently updated. Potentially confusing in some cases and very powerful in other cases.

Makes it basically impossible to spot intentional and unintentional use. If I want to use multiple signals for one effect, I can create this as one signal.

  1. Set up a new signal binding after unregistering the previous signal binding.

Sounds convenient, but again something might fall off unintentionally. So maybe also useful
if the caller can request this, but it’s not the default.

  1. Throw IllegalStateException just as for setVisible(true).

See above.

Method naming

  1. One obvious candidate would be to stick to the same name setVisible(true) and setVisible(someSignal). This might be problematic if the “right” answer from the previous questions would make the methods have significantly different semantics.

Isn’t this problematic for say setFoo(Foo foo) and setFoo(Signal<Foo> signalFoo) when both things allow null? Also once the semantics (“bind-then-set” and “bind-and-bind-again”) are settled, this overload set as one single verb even more.

  1. Different semantics can be addressed with a different name, e.g. bindVisible(someSignal). This does instead have a negative impact on discoverablity since it might be that not all regular setters have corresonding “bind” methods (you would use those through ComponentEffect instead) and it would require an extra step to check if it’s available for your case.

I’d prefer an explicit naming scheme like this. The question is, how used bind already is and are there places, where new functions like this are not possible, because the same name is already claimed (e.g. form binder comes to mind; a naive symbol search brings up some hits also in vaadin-dev, which most likely are not problematic).

  1. Show the semantic difference but use a familar prefix: setVisibleBinding(someSignal). The main drawback here is that the name is slightly more verbose.

Might be OK and makes maybe more sense with whatever decision will be made for the bind-vs-set behaviour. Yet I’d prefer something along the lines of setVisibleBySignal or setVisibleFromSignal.

All that said… to finish with something (thought) provoking:

Why bother at all with adding all those methods?

I imagined, that Component.effect and Component.bind would become parts of the regular components anyway (e.g. Mix-In via some high level interface as default). The only problem with this is, that “resource management” (e.g. Registration.close) is still the problem of the developer. So if that part is handled as well, that would leave less room to make errors. So generic use is possible. Not much interface of components changes.

Once you start adding additional methods, you will add lots of stuff.

  • So how to decide, that when to add the new methods for a field in a component? Always, if there is a getter/setter? Popular vote? Request? Can this be generated?
  • How easy is it for authors of components (e.g. in the directory) to add this in their components as well to be on par with the core?
  • Will this become a minefield of (in)convenience functions? E.g. will there be a bindWidth(Signal<String>) or a bindWidth(Signal<Number> width, Signal<Unit> unit) or rather one with a proper data class? What’s with “widthFull” and “widthUndefined”? What of setText or anything, that takes a String; will we be forced into format whether we like it or not? (e.g. I’d prefer just one type, and I’d nuke format).

More provocation: make Flow signal only. Is this the road we are moving on? Will signals be the backing of everything eventually anyway? Is thinking about threading them in now the wrong thing to think about: what would be ideal for such a system and how to make the old behaviour work (e.g. setVisible(Bool) would just set a constant signal in the new machinery).

7

Thanks for your insights!. You provide external validation for the things that I suspected but was on purpose a bit vague about just to avoid confirmation bias. And then you also add additional depth in areas that I have so far been ignoring.

The main constraint is the component APIs. If we keep setVisible(false) working, then it doesn’t matter too much how that’s managed internally. If we instead go all-in and require changing that to bindVisible(constantSignal(false)), then we would have lots of painful breaking changes ahead of us.

I suspect we might want to eventually change the internals to enable an optional “stateless mode” where views can be singletons and all the actual component state is in non-constant signals that can transparently delegate to e.g. Redis or request/response parameters. But that’s still far into the future (if ever) and I choose to make the simplifying assumption that it won’t impact the high-level API design on the table right now.

I think it will be useful to have three distinct constructors for text-centered components: new Span("static text"), new Span(textSignal) and new Span("a: %s, b: %s", aSignal, bSignal). I assume the third case is common enough that it’s worth having this overload rather than new Span(Signal.computed(() -> "a: %s, b: %s".formatted(aSignal.value(), bSignal.value())). A helper on Signal could simplify that to new Span(Signal.formatted("a: %s, b: %s", aSignal, bSignal)) which is relatively short but probably not discvoverable enough for beginners. It would then also be expected to have corresponding setText or bindText methods.

It would be really cool to have an extension function on String to enable "a: %s, b: %s".signalFormat(aSignal, bSignal) but this is Java and not Kotlin. I’m still waiting for a replacement to the String Templates JEP but I don’t think we should have compromises in our API in anticipation of something that won’t be mainstream for many years. With the old JEP, it could have been like new Span(Signal.FORMAT."a: \{aSignal}, b: \{bSignal})". I hope there will be a future JEP that allows a syntax like new Span("a: \{aSignal}, b\{bSignal}").

Mostly convenience since these would be very common operations. section.bindVisible(signal) is fewer tokens than any other alternative.

  • signal.bind(section::setVisible) is almost as short but it cannot do resource management unless we introduce a potentially confusing non-local mechanism
  • section.bind(signal, Component::setVisible) is the shortest format that I’ve come up with that can do resource management

We can provide helpers in Element to give consistent behavior for anything that delegates directly to setProperty, setAttribute or setText. Cases with more explicit state management would need a little bit of additional code that could also be partially supported by some helpers in Component or Element .

I think we should provide shorthand methods only for the cases determined to be most common according to a benevolent dictator. There should be something like component.bind(signal, MyComponent::setFoo) for the long tail and compnent.effect(callbackDoingWhatever) for the weird cases that involve multiple parameters.

One tricky detail here that makes me uncertain about the method semantics is the way we can’t (again without introducing new non-local mechanisms) know whether there’s a binding created using a generic construct (since identical method references are not equals with each other) which in turn means that we cannot make the regular setter do anything special if a binding is active (e.g. throw or disable the binding). This means that section.bindVisible(signal) would behave differently than section.bind(signal, Component::setVisible) with regards to section.setVisible(true). This might be a source of confusion but I’m not sure if it’s big enough to worry about.

The one reason for something “astonishing” that I have heard about would be to ease the transition for existing code where it might not be feasible to phase out all setVisible(true) calls in one go.

Nice catch. Introducing a new setFoo(singleArgument) method would lead to compilation errors for any existing code written as setFoo(null) so this is simply not a workable option.

We would still break an explicit new Span(null) with the additional constructors that I imagine but I think that’s rare enough to be acceptable.

8

I’ve been alluding to some potential “non-local” mechanisms that would enable a more convenient syntax. A have dug slightly deeper into what this would imply and come to the conclusion that it’s a definite no-go.

One tempting case would be to enable a short and sweet signal.bind(component::setText) syntax for creating generic bindings without dedicated methods in all classes. For this to make sense, we would need a way of doing automatic resource management, i.e. so that the signal instance can automatically stop referencing the component instance when the component is detached. The challenge with the syntax is that there’s no direct way of “extracting” component from the Consumer<T> instance that the method receives. Furthermore, you would expect signal.bind(value -> component.setText(value)) to behave in exactly the same way. (Yes, I’m familiar with the fragile serialization trick and how Project Babylon might give a better solution in the future.)

This could be “fixed” by making setText register itself with a thread local so that a detach listener could be added after running the callback. Something like Effect.getCurrent().ifPresent(effect -> effect.addLifeCycleOwner(this)). This would, however, be a disaster waiting to happen whenever there’s conditional logic inside the callback since that might lead to some resources not being detected:

signal.bind(value -> {
  if (value.length() > 5) component.setText(value);
});

The callback will always have a reference to the component instance but it will be detected only in some cases. In other words, the mechanism would give the developer the false impression that they don’t need to care about resource management but there would in practice be some non-trivial cases where memory could still leak.

9

The challenge with the syntax is that there’s no direct way of “extracting” component from the Consumer instance that the method receives

Just thinking loud, but how about all our void setters in the components start to return the component itself, like for instance the grid column methods do?

I know, it’s a lot of refactoring and there will be some edge cases where it could lead to issues, since some interfaces need an additional generic type for the return value, but it should solve that issue, if I’m not wrong.

It would also additionally provide chaining of mutliple method calls for a single component, like

textField
    .setValue("Hello")
    .setRequires(true)
    .setEnabled(false);

Maybe something for V25.

*edit
As a starter, the methods could return Component for now, if there is no information about the type to return. Then it would not be major breaking change and could be introduced earlier. HasValue for instance already knows the owning type.

10

That’s a can of worms that I’d rather avoid. It all starts from the fact that components are supposed to be subclassed. This gives the problem of making all the methods return the subclass type so that the ordering of component.setOwnMethod(a).setInheritedMethod(b) won’t matter. This is a problem already if the return type is just Component since then component.setOwnMethod(a).setVisible(b) works but it breaks down if you try to change the ordering.

11

This has been avoided, but still coming up regularly… Already the use of annotations in Vaadin components makes the inheritance often infeasible, when you cannot overwrite those values e.g. changing the loaded client-side implementation. Have you experienced the same?

Anyway, the return value API would not make the situation much worse than it is, and as upside it would offer a modern way of configuring the stock components like @Stefan.27 shows. Builders are also common way to solve this. Maybe could be offered as generated add-on for those wanting it?

For example:

Component c = Component.builder()
                       .timeout(Duration.ofSeconds(5))
                       .retries(3)
                       .build();

instead of

component.setTimeout(Duration.ofSeconds(5))
         .setRetries(3);
12

That’s a more infrequent problem. And I would claim that it’s a symptom of lack of modularity in the client-side logic if you need to avoid loading the original script rather than just supplementing it with some additional script.

A builder approach is technically feasible but it comes with two tradeoffs:

  1. Any new component feature needs yet another new method since you need to have setTimeout in the component and timeout in the builder class. You must remember to also update the builder if you e.g. add a new overload to the component. It really needs to be automatically generated to be feasible.
  2. It increases the burden on add-on developers.
13

Going on a side track here, but it’s also questionable whether a separate builder of the type that can easily be generated gives any benefits over the {{ pattern:

MyComponent c = MyComponent.builder()
                       .timeout(Duration.ofSeconds(5))
                       .retries(3)
                       .build();

vs

MyComponent c = new MyComponent() {{
  setTimeout(Duration.ofSeconds(5));
  setRetries(3);
}};
14

I’ve explored what would be required to make it easy for component implementations to provide a consistent API for features that can be configured using signals based on these requirements:

  • The setter (e.g. setVisible(boolean)) throws if a signal binding is active.
  • The bind method (e.g. bindVisible(Signal<Boolean>) throws if a signal binding is active.
  • The getter, (e.g. isVisible()) reads from the signal if there’s a binding and otherwise reads the previously applied value. This is important since the effect doesn’t apply changes from the signal while the component is detached.
  • There’s optionally a method for checking whether a signal binding is active (e.g. isVisibleBound()).
  • Passing null to the bind method clears any current binding and sets the value based the signal value. (I considered returning a Registration instead but that complicates usage without any big benefit.)
  • Don’t use more memory than today when properties are set with a regular value rather than a signal.

It can work like this transparently for any component API that delegates directly through getElement() (i.e. setProperty, setAttribute, or setText) just by adding a bind method to the component that delegates to a new getElement().bindProperty("name", signal); method (or corresponding new methods for attributes and text).

It gets more interesting for component features that manage the state as an instance field in the component class and then either uses that only for handling server-side events or applies the state to the client through e.g. executeJs. The requirement to not use additional memory while signals are not used leads to using “polymorphism” for the instance field so that it either contains the actual value or an object representing the signal binding and the type of the field is thus just Object (and this still means that more memory is used for primitive fields since e.g. Integer users more memory than int). I implemented some static helper methods to help deal with this “polymorphic” instance field so that each component method can be a one-liner rather than having an explicit instanceof check in each component method.

A simple component using this approach would look like this:

@Tag("span")
public class MyComponent extends Component {
    // String or SignalBinding<String, MyComponent>
    private Object textOrBinding;

    private void applyText(String text) {
        // Just to have something else than setting an element property
        getElement().executeJs("this.textContent = $0", text);
    }

    public String getText() {
        return SignalBinding.getValue(textOrBinding);
    }

    public void setText(String text) {
        textOrBinding = SignalBinding.setValue(textOrBinding, this, text,
                MyComponent::applyText);
    }

    public void bindText(Signal<String> textSignal) {
        textOrBinding = SignalBinding.bindValue(textOrBinding, this, textSignal,
                MyComponent::applyText);
    }

    public boolean isTextBound() {
        return SignalBinding.isBound(textOrBinding);
    }
}
15

The textOrBinding member would sure look better with a union type… is there anything better Java can (reasonably) offer, or is the plain Object the way it has to go?

16

I’m not aware of anything better with Java if we want to avoid increasing memory consumption for non-signal cases.

If memory isn’t a concern, then the instance field could be private final ComponentProperty<String> textProperty = ...; and then e.g. the setter implementation can be reduced to textProperty.set(text); But then we increase memory consumption by at least 16 bytes (assuming compressed OOPs but nothing from Lilliput) for each such field in each component instance. Most component features map directly to Element features so there aren’t actually too many such fields at least in the components that are used in large quantities (e.g. VerticalLayout, Span and Div have no own instance fields).

Another option for hiding the Object field would be to have static property descriptors that are used as lookup keys in an internal compact structure of Object values. Usage would then be like private static Property<String> TEXT = ...; and setState(TEXT, text); where setState is defined in a base class. A regular HashMap<Property<?>, Object> is not very compact in memory but we could use a Object[] as long as the property-to-index mapping for each component class could be stable. That is probably possible if we require that the property descriptors are static fields that we can look up with reflection from the component class (and its super classes). Memory overhead compared to regular instance fields would then be a constant 16 bytes per component instance instead of per signal-supporting instance field. The problem is that we’d want to put that field in Component so that you could use the feature also if you subclass some component that doesn’t need it. That means that every component instance would grow by at least 4 bytes for a pointer even if the value is null.

17

The outcome of this discussion are now summarized as part of RFC: Signal patterns for Flow components.