fix: add resource pattern for file-routes.json #2396
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
taefi
previously requested changes
May 10, 2024
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: |
|
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 |
|
|
I pushed the changes needed for a Vaadin/Hilla 24.4 applications to work with the dynamic menu. |
platosha
approved these changes
May 20, 2024
vaadin-bot
pushed a commit
that referenced
this pull request
May 20, 2024
* 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>
platosha
added a commit
that referenced
this pull request
May 21, 2024
) * 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This should fix that
file-routes.jsoncannot be loaded from/META-INF/VAADIN/when Hilla application is executed as native image.Fixes #2395
Type of change
Checklist