Skip to content
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: add endpoint detection #2938

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: add endpoint detection #2938

wants to merge 6 commits into from

Conversation

tepi
Copy link
Contributor

@tepi tepi commented Nov 28, 2024

Add check for if endpoint generator needs to run

Fixes vaadin/flow#20289
Fixes vaadin/flow#18800

@tepi
Copy link
Contributor Author

tepi commented Nov 29, 2024

Needs to be tested along with vaadin/flow#20567 in order to compile

Comment on lines 18 to 20
return !classFinder.getAnnotatedClasses(Endpoint.class).isEmpty()
|| !classFinder.getAnnotatedClasses(BrowserCallable.class)
.isEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

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

This check should probably exclude Hilla internal annotated classes, like SignalsHandler

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent point, indeed, without this Flow would always try to generate the endpoints unless one completely excludes Hilla.
Something similar is needed:

        ClassFinder classFinder = options.getClassFinder();
        return Stream.concat(
            classFinder.getAnnotatedClasses(Endpoint.class).stream(),
                classFinder.getAnnotatedClasses(BrowserCallable.class).stream())
            .anyMatch(annotatedClass -> !annotatedClass.getPackage().getName()
                .startsWith("com.vaadin.hilla"));

Comment on lines 49 to 50
Assertions.fail("Class " + clazz.getName()
+ " is annotated with @BrowserCallable or @Endpoint, but missing @InternalBrowserCallable.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the assertion message is not correct here: in this case, all annotatedClasses will have @InternalBrowserCallable annotation, but they might miss @BrowserCallable or @Endpoint

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the messages in the two tests are inverted

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they are inverted indeed, thanks for noticing it 👍

@mshabarov mshabarov force-pushed the feat/endpoint-detection branch from ed89b41 to 5e2e68c Compare December 19, 2024 13:28
@mshabarov mshabarov requested a review from platosha December 20, 2024 08:38
@mshabarov
Copy link
Contributor

mshabarov commented Dec 20, 2024

This should be ready for review now, tried locally and it seems to pass on existing tests and new ones.
Requires review and a suitable time to merge flow and hilla parts simultaneously.

Comment on lines +22 to +26
return Stream.concat(
classFinder.getAnnotatedClasses(Endpoint.class).stream(),
classFinder.getAnnotatedClasses(BrowserCallable.class).stream())
.anyMatch(annotatedClass -> !annotatedClass
.isAnnotationPresent(InternalBrowserCallable.class));
Copy link
Contributor

Choose a reason for hiding this comment

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

Searching in the classpath for annotated classes does not necessarily match the reality of the endpoints in a Hilla application.

Until 24.6, the list of packages where to search for endpoints is configurable and, if not configured, defaults to search inside target/classes for backwards compatibility.

Starting from 24.7, endpoints must be Spring beans (which was the case anyway 99% of the times).

So, in both cases the presence of an annotated class does not necessarily mean that an endpoint will be compiled to TypeScript and used in communication with clients.

We have a registry of endpoints, but of course that comes probably too late for your needs.

Also, we should define a common way to skip internal endpoints, to use also in

endpointBeans.keySet().stream()
.filter(name -> !name.equals(SIGNALS_HANDLER_BEAN_NAME))
.findAny().ifPresent(name -> HillaStats.reportHasEndpoint());

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, this is where we define the list of endpoints using Spring ApplicationContext:

public static List<Class<?>> findBrowserCallables(
EngineConfiguration engineConfiguration,
ApplicationContext applicationContext) {
return engineConfiguration.getEndpointAnnotations().stream()
.map(applicationContext::getBeansWithAnnotation)
.map(Map::values).flatMap(Collection::stream)
// maps to original class when proxies are found
// (also converts to class in all cases)
.map(AopProxyUtils::ultimateTargetClass).distinct()
.collect(Collectors.toList());
}

Copy link
Contributor

@platosha platosha left a comment

Choose a reason for hiding this comment

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

What should happen when the user removes last Hilla browser callable? I think, ideally, we should remove previously generated files then. And it's maybe better for such cleanup implementation to be in Hilla.

I'm thinking of taking a different approach here: Hilla “generator” are really two tasks:

  • TaskGenerateOpenAPI is the Java parser, and is purely in Java. It's basically safe to leave it always on, since there aren't Node dependencies.
  • TaskGenerateEndpoint is the TypeScript generator, which actually requires Node and npm dependencies). This one is the one that needs caution.

Instead of trying to skip both of them in Flow, we could keep both of them enabled, but leave it up to TaskGenerateEndpoint implementation to skip invoking Node when not needed.

This way, introducing a separate detection logic, Hilla would find the endpoints using its regular approach. It would also make it possible to implement a proper cleanup in the TaskGenerateEndpoint task, removing any previously generated files listed in the generated-file-list.txt.

@tepi
Copy link
Contributor Author

tepi commented Jan 17, 2025

@platosha Just to confirm I understood correctly: In Flow, in com.vaadin.flow.server.frontend.NodeTasks#addEndpointServicesTasks we should always add both Hilla tasks, and Hilla will then decide what needs to be executed? If so, I can rework the Flow PR to do this, and someone from Hilla team can change this PR / create a new PR that will do the detection using existing Hilla approach.

@platosha
Copy link
Contributor

platosha commented Jan 17, 2025

@platosha Just to confirm I understood correctly: In Flow, in com.vaadin.flow.server.frontend.NodeTasks#addEndpointServicesTasks we should always add both Hilla tasks, and Hilla will then decide what needs to be executed? If so, I can rework the Flow PR to do this, and someone from Hilla team can change this PR / create a new PR that will do the detection using existing Hilla approach.

@tepi Correct. Sounds good, let's do so.

@tepi
Copy link
Contributor Author

tepi commented Jan 17, 2025

Updated vaadin/flow#20567 to execute both Hilla tasks whenever Hilla is available on the class path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants