Throw error when using signal.get() inside bindChildren callback#23845
Open
Throw error when using signal.get() inside bindChildren callback#23845
Conversation
Wrap the child factory callback in bindChildren with a UsageTracker deny-registrar that throws DeniedSignalUsageException when signal.get() is called directly. This prevents accidental eager reads that create unwanted dependencies at the effect level instead of per-component reactive bindings. Users should use peek() for one-time reads or pass the signal to a component that creates its own reactive binding (e.g. new Span(() -> signal.get())). Fixes #23841
Test Results 1 384 files ±0 1 384 suites ±0 1h 33m 40s ⏱️ + 2m 28s For more details on these failures and errors, see this check. Results for commit bc91323. ± Comparison against base commit 6a63221. ♻️ This comment has been updated with latest results. |
Legioth
reviewed
Mar 12, 2026
flow-server/src/test/java/com/vaadin/flow/dom/ElementEffectTest.java
Outdated
Show resolved
Hide resolved
Propagate DeniedSignalUsageException immediately to the caller instead of routing it through the session error handler. This gives developers an immediate stack trace at the call site when they mistakenly use signal.get() inside a bindChildren child factory. - ElementEffect.executeAction: catch DeniedSignalUsageException separately and re-throw before the general RuntimeException handler - Effect constructor: re-throw RuntimeExceptions on first run so they propagate out of the synchronous initial execution - Update test to use assertThrows instead of error handler assertion
Replace the broad first-run RuntimeException re-throw with a targeted catch for DeniedSignalUsageException. The previous approach broke EffectTest because it re-threw all RuntimeExceptions on first run, but raw Effect usage expects exceptions to be routed to the uncaught exception handler even on the initial run.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wrap the child factory callback in bindChildren with a UsageTracker deny-registrar that throws DeniedSignalUsageException when signal.get() is called directly. This prevents accidental eager reads that create unwanted dependencies at the effect level instead of per-component reactive bindings.
Users should use peek() for one-time reads or pass the signal to a component that creates its own reactive binding
(e.g. new Span(() -> signal.get())).
Fixes #23841