-
Notifications
You must be signed in to change notification settings - Fork 55
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
fix: use Qualifier
to force bean dependency
#3098
fix: use Qualifier
to force bean dependency
#3098
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3098 +/- ##
=======================================
Coverage 92.60% 92.60%
=======================================
Files 85 85
Lines 3164 3164
Branches 775 775
=======================================
Hits 2930 2930
Misses 183 183
Partials 51 51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dependsOn
to force bean dependencydependsOn
to force bean dependency
@@ -154,6 +155,7 @@ private static ObjectMapper createDefaultEndpointMapper( | |||
* @return the endpoint invoker | |||
*/ | |||
@Bean | |||
@DependsOn("hillaEndpointObjectMapper") |
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.
This ensures that the bean named ”hillaEndpointObjectMapper” is created before this bean, right? But what ensures that that particular mapper is used here and not some other ObjectMapper in the project?
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 a qualifier
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 the same qualifier be used here
hilla/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/config/SignalsConfiguration.java
Line 39 in 6dd555b
public SignalsConfiguration(EndpointInvoker endpointInvoker, |
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 think so
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.
is the whole DependsOn
now unnecessary, as it requires the specific Hilla object mapper?
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 would imagine it is
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 can't find any confirmation that @Qualifier
guarantees creation order. For what it worths, ChatGPT says that @DependsOn
is still required. Unless we find confirmation, I'd keep it
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.
If the Hilla endpoint mapper is not created before this is called, what would be passed as the parameter?
…on-order-of-EndpointObjectMapper-and-endpointInvoker
…on-order-of-EndpointObjectMapper-and-endpointInvoker
dependsOn
to force bean dependencyQualifier
to force bean dependency
…on-order-of-EndpointObjectMapper-and-endpointInvoker
…on-order-of-EndpointObjectMapper-and-endpointInvoker
…on-order-of-EndpointObjectMapper-and-endpointInvoker
Quality Gate passedIssues Measures |
This ticket/PR has been released with Hilla 24.7.0.alpha6 and is also targeting the upcoming stable 24.7.0 version. |
* Use `dependsOn` to force bean dependency * Add qualifier * Add qualifier * Remove `@DependsOn` * Update test to match new code --------- Co-authored-by: Artur <[email protected]>
fix: use `Qualifier` to force bean dependency (#3098) * Use `dependsOn` to force bean dependency * Add qualifier * Add qualifier * Remove `@DependsOn` * Update test to match new code --------- Co-authored-by: Luciano Vernaschi <[email protected]> Co-authored-by: Artur <[email protected]>
Fixes #3092