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: improved endpoint detection #21009

Merged
merged 13 commits into from
Feb 25, 2025
Merged

feat: improved endpoint detection #21009

merged 13 commits into from
Feb 25, 2025

Conversation

tepi
Copy link
Contributor

@tepi tepi commented Feb 19, 2025

Fixes #20983

Copy link

github-actions bot commented Feb 19, 2025

Test Results

1 169 files  ± 0  1 169 suites  ±0   1h 32m 43s ⏱️ - 3m 16s
7 774 tests + 1  7 717 ✅ + 3  57 💤 ±0  0 ❌  - 2 
8 105 runs   - 29  8 040 ✅  - 26  65 💤  - 1  0 ❌  - 2 

Results for commit dc67e73. ± Comparison against base commit f849d20.

♻️ This comment has been updated with latest results.

@tepi tepi requested a review from mshabarov February 21, 2025 09:15
@tepi
Copy link
Contributor Author

tepi commented Feb 21, 2025

Needs to be tested with a Hilla build from this PR branch

/**
* Checks if Hilla is available and Hilla endpoints are used in the project.
*
* @return {@code true} if Hilla is available and Hilla views are used,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return {@code true} if Hilla is available and Hilla views are used,
* @return {@code true} if Hilla is available and Hilla endpoints are used,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -319,6 +319,9 @@ private void addEndpointServicesTasks(Options options) {
if (!EndpointRequestUtil.isHillaAvailable(options.getClassFinder())) {
return;
}
if (!EndpointRequestUtil.areHillaEndpointsUsed(options)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if views are there but no endpoints yet?
I'd do it in this way:

  1. If Hilla not available - return
  2. if endpoints are used or views are used - continue, else - return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the logic

Comment on lines +565 to +568
if (endpointGeneratorTaskFactory != null) {
annotations.addAll(endpointGeneratorTaskFactory
.getBrowserCallableAnnotations());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be covered by tests, so needs a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.1.0 labels Feb 24, 2025
@vaadin-bot vaadin-bot added +0.1.0 and removed +1.0.0 labels Feb 24, 2025
@mshabarov
Copy link
Contributor

Works as expected for me.
Need to figure out why tests are failing.

@mshabarov mshabarov merged commit 530e757 into main Feb 25, 2025
26 checks passed
@mshabarov mshabarov deleted the feat/endpoint-detection-v2 branch February 25, 2025 07:54
mshabarov added a commit that referenced this pull request Feb 25, 2025
tepi pushed a commit that referenced this pull request Feb 25, 2025
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.7.0.alpha12 and is also targeting the upcoming stable 24.7.0 version.

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

Successfully merging this pull request may close these issues.

BrowserCallable-aware package handling in Flow
3 participants