-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat!(generator): load endpoints from Spring components and stop using Maven/Gradle at each generation #2616
base: main
Are you sure you want to change the base?
Conversation
30c2e88
to
236efd5
Compare
9d4988c
to
a917323
Compare
d7e3b47
to
4023bde
Compare
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.
Would you consider the introduction of an interface to make the endpoint search independent of Spring AOT?
Something like
@FunctionalInterface
public interface EndpointProvider {
List<Class<?>> findEndpoints(EngineConfiguration configuration) throws ExecutionFailedException;
}
and making AotEndpointFinder
implement it.
The EngineGenerateMojo
mojo would then have a @Paramter
annotated EngineConfiguration.EndpointProvider
field, defaulting to AotEndpointFinder
and would simply do var endpoints = endpointProvider.findEndpoints(conf);
.
This should allow implementing custom endpoint finders not based on Spring by configuring the plugin; for example the implementation can search a Jandex index.
<endpointProvider implementation="org.vaadin.test.MyEndpointPRovider"></endpointProvider>
Actually, I don't know how to make it work in Gradle, but I suppose this should be possible.
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.
Another question: why does it need to run SpringApplicationAotProcessor
to find endpoints? Couldn't the scan be done with Flow ClassFinder
?
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 have the general feeling that making Hilla depend more on Spring will actually make easier to isolate Spring dependency. For example, at the moment Spring is heavily used in the connector, but not in the generator.
The whole point of this PR is to let Spring decide which endpoints are available. A single provider must be trusted both in the code generation and in the controller which runs in the application. When they will be coherent, it will be easier to swap the "source of truth".
Concerning the ClassFinder
, while I don't know it well, I guess it's just another way to walk the classpath searching for annotated classes. Correct me if I'm wrong, but that's what I'm trying to avoid here: finding too many endpoints, that are in the classpath, but not loaded by Spring.
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.
Got it. Thanks for clarification.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2616 +/- ##
=======================================
Coverage 92.64% 92.64%
=======================================
Files 83 83
Lines 2842 2842
Branches 739 739
=======================================
Hits 2633 2633
Misses 158 158
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. 🚨 Try these New Features:
|
Quality Gate passedIssues Measures |
Removes the need of calling Maven (or Gradle) at each code generation during development.
Endpoints are now searched between the available Spring Beans. When building for production mode, since the application is not running, some code provided by Spring is run to be able to get a list of beans as if the application was running.
The generator configuration through Maven is no longer possible, since the application can be run directly as a main class. The idea is to provide it as a singleton, which should become a Spring bean. At the moment, it's just an ordinary class and cannot be configured externally, but since that's internal code, the change can happen in future, when some configuration means has been chosen.
Gradle support is work in progress, not tested (doesn't even compile, actually).
Many tests are touched by these changes and some no longer work, so further fixes are needed, to the tests themselves to accommodate for the new workflow, or to the code if those tests should keep passing.
The new
annotations
package should probably be renamed as it also contains a type.