-
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
fix: add resource pattern for file-routes.json #2396
fix: add resource pattern for file-routes.json #2396
Conversation
I followed your recommendation @taefi and made the suggested change. Unfortunately, my knowledge about the Hilla project-, build- and test-setup is not sufficient, which means, that I don't know how to verify that the change fixes the issue and I don't know how to add a test case for this particular fix. |
@rbrki07 while we're looking at this issue, if you're interested in testing your changes locally, it should not be complicated:
For overriding the dependency of the changed module in your playground project, use the latest publicly available version of the Hilla/Vaadin (at this time <project>
<properties>
<java.version>17</java.version>
<vaadin.version>24.4.0.beta1</vaadin.version>
</properties>
<!-- skipped listing the repositories and pluginRepositories as you already have the prereleases -->
<dependencies>
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>hilla-endpoint</artifactId>
<version>24.5-SNAPSHOT</version>
</dependency>
<!-- make sure the dependency of the changed module(s) listed on top of the vaadin managed dependencies -->
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>vaadin-spring-boot-starter</artifactId>
</dependency>
<!-- rest of the dependencies -->
<dependencies/>
</project> Maven uses the timestamp of the latest snapshot (either built locally or coming from our regular builds). So, make sure to build Hilla locally right before testing it in your playground project to avoid confusions about which snapshot is in use. Hope this helps. |
packages/java/engine-core/src/main/java/com/vaadin/hilla/engine/EngineConfiguration.java
Outdated
Show resolved
Hide resolved
Thank you @taefi. I will try your instructions and check it locally. I will also apply your suggested changes. I will come back to this pr in a few days, because I'm AFK for a few days. |
I followed your instruction @taefi, and I was able to test the change locally 🥳 I tried different variations, but unfortunately none of them worked as desired yet. I will come back to this soon. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2396 +/- ##
=======================================
Coverage 95.14% 95.14%
=======================================
Files 66 66
Lines 4512 4512
Branches 650 650
=======================================
Hits 4293 4293
Misses 178 178
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for your contribution @marcushellberg! To test your change, I created a new project using Compiling the app into a native image using
Can you reproduce this? |
Adding a runtime hint for
|
Adding this to hints.resources().registerPattern("file-routes.json");
hints.reflection().registerType(ClientViewConfig.class, MemberCategory.values());
hints.reflection().registerType(ClientViewMenuConfig.class, MemberCategory.values());
hints.reflection().registerType(AvailableViewInfo.class, MemberCategory.values()); results in a startup of a native compiled Hilla app without errors and I even see the menu items 😍
Invoking
|
Progress! I wonder what has changed in calling endpoints. It used to work. Have we changed how endpoints are being called? |
I just realized that |
The |
I don't recall any change in that zone. The stacktrace points to this method invocation that uses the reflection API, but the code hasn't changed since early 2022. |
This is the code that is supposed to register all needed types for endpoints: hilla/packages/java/endpoint/src/main/java/com/vaadin/hilla/springnative/HillaHintsRegistrar.java Lines 56 to 79 in 8f142a3
|
Well, looking at the above code, it tries to read the open api json from Then, obviously it fails to call the endpoint's method at runtime since no methods has been registered during the build. What is weird is:
|
It seems that the |
Nope :( It is happening again (though I had a successful native build earlier): I suspected that timing might be the issue, for instance when that the open api json is not yet available during the native build, but it created during the production build much earlier than the native build is getting started. |
I've noticed a weird pattern of having "Resource /hilla-openapi.json is not available" during the native build: |
Well, as suspected, adding a log shows when Still, it could be investigated that why a change of name in |
Quality Gate passedIssues Measures |
I pushed the changes needed for a Vaadin/Hilla 24.4 applications to work with the dynamic menu. |
* fix: add resource pattern for file-routes.json * apply suggestions from taefi * Register hints for META-INF/VAADIN/* and ClientViewConfig.class * fix hints for deserializing file-routes properly --------- Co-authored-by: Marcus Hellberg <marcus@vaadin.com> Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com>
) * fix: add resource pattern for file-routes.json (#2396) * fix: add resource pattern for file-routes.json * apply suggestions from taefi * Register hints for META-INF/VAADIN/* and ClientViewConfig.class * fix hints for deserializing file-routes properly --------- Co-authored-by: Marcus Hellberg <marcus@vaadin.com> Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com> * import AvailableViewInfo from hilla * import ClientViewMenuConfig from hilla instead of MenuData --------- Co-authored-by: René Wilby <rbrki07@gmail.com> Co-authored-by: Marcus Hellberg <marcus@vaadin.com> Co-authored-by: Soroosh Taefi <taefi.soroosh@gmail.com> Co-authored-by: Anton Platonov <platosha@gmail.com>
Hi @taefi,
|
Ah. Good that you reminded of that. It's not fixed, but needs to be fixed #2461 |
This ticket/PR has been released with Hilla 24.5.0.alpha1 and is also targeting the upcoming stable 24.5.0 version. |
Description
This should fix that
file-routes.json
cannot be loaded from/META-INF/VAADIN/
when Hilla application is executed as native image.Fixes #2395
Type of change
Checklist