Skip to content

feat: SignalInput — read a server-side Signal at trigger fire time#24428

Open
Artur- wants to merge 1 commit into
mainfrom
trigger-signal-input
Open

feat: SignalInput — read a server-side Signal at trigger fire time#24428
Artur- wants to merge 1 commit into
mainfrom
trigger-signal-input

Conversation

@Artur-
Copy link
Copy Markdown
Member

@Artur- Artur- commented May 22, 2026

Adds SignalInput(Component owner, Signal signal) under com.vaadin.flow.component.trigger.internal. A component-scoped Signal.effect mirrors the signal value to a uniquely-named JS property on the owner element via executeJs; the input's rendered expression reads that property at fire time, so actions such as CopyTextToClipboardAction can use a server-side signal as their value source without first projecting it onto a DOM property.

Includes unit tests covering the rendered expression, push-on-change, and per-input property naming, plus an IT that copies a ValueSignal, mutates it from a separate button, and verifies the next copy sees the updated value.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 22, 2026

Test Results

 1 422 files  +1   1 422 suites  +1   1h 22m 50s ⏱️ + 3m 4s
10 015 tests +5   9 947 ✅ +5  68 💤 ±0  0 ❌ ±0 
10 487 runs  +5  10 418 ✅ +5  69 💤 ±0  0 ❌ ±0 

Results for commit 5f2d8be. ± Comparison against base commit d9afcc4.

♻️ This comment has been updated with latest results.

@Artur- Artur- requested a review from tltv May 25, 2026 10:07
ui.getElement().appendChild(owner.getElement());

ValueSignal<String> signal = new ValueSignal<>("first");
new SignalInput<>(owner, signal);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It bothers a bit that constructor call generates effect and its initialization also calls executeJs that sets the property value.. and all that just by creating SignalInput object. Feels a bit too early, what if input is not even used.
Maybe this could be postponed to triggers(...) calls instead, meaning it could be moved to appendExpression method? Or new optional lifecycle method called initExpression.

Technically signal could be stored to SignalBindingFeature to bound it for garbage collection of the owner element with:

owner.getElement().getNode().getFeature(SignalBindingFeature.class)
   .setBinding(propertyName, signal);

and read with:

Signal<T> signal = owner.getElement().getNode().getFeature(SignalBindingFeature.class)
   .getSignal(propertyName);

where owner would go to new private field.

Benefits:

  • signal effect and its initialization runs on trigger(...) instead where inputs are actually used.
  • SignalInput is similar as other inputs. Like a pojo without a trigger usage.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved to appendExpression

*/
public class SignalInput<T> extends Action.Input<T> {

private static final AtomicLong PROPERTY_INDEX = new AtomicLong();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a comment explaining why the counter never resets. Long is probably "long" enough, but without any comment confirming that, it makes one wonder if some session-per-request scenario, or devmode hotdeploy could explode it. And in that case UUID could work nicer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added comment

Adds SignalInput<T>(Component owner, Signal<T> signal) under
com.vaadin.flow.component.trigger.internal. A component-scoped
Signal.effect mirrors the signal value to a uniquely-named JS
property on the owner element via executeJs; the input's rendered
expression reads that property at fire time, so actions such as
CopyTextToClipboardAction can use a server-side signal as their
value source without first projecting it onto a DOM property.

Includes unit tests covering the rendered expression, push-on-change,
and per-input property naming, plus an IT that copies a ValueSignal,
mutates it from a separate button, and verifies the next copy sees
the updated value.
@Artur- Artur- force-pushed the trigger-signal-input branch from 4b4b5df to 5f2d8be Compare May 26, 2026 20:23
@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants