feat: add collectMenuItemsList() to MenuRegistry #19952
Conversation
| value.register(), value.menu(), value.children(), | ||
| value.routeParameters(), value.flowLayout()); | ||
| }).sorted(getMenuOrderComparator( | ||
| Collator.getInstance(Locale.forLanguageTag("en-US")))).toList(); |
There was a problem hiding this comment.
This might be a problem with GraalVM and their idea to have as small as possible executables.. Collator.getInstance() might be more random.. but probably safer
There was a problem hiding this comment.
Am I understanding this right that you mean that usage of Collator instance for en-US could increase memory consumption compared to just using the default provided with Collator.getInstance()?
There was a problem hiding this comment.
Correct, because GraalVM only includes locales / languages that are required by the application - if the application is e.g. completely in german, the above language might not be available - or GraalVM adds it if needed (this part I'm not sure.. I just remember seeing issues regarding this topic in the past)
There was a problem hiding this comment.
en-US matches with how Hilla is doing it in Typescript here
We should sort same way in both places and currently it's hard coded in Hilla. If this causes trouble, it can be avoided by adding optional Locale parameter in the API. Default can be en-US. I'll add another method collectMenuItemsList(Locale).
There was a problem hiding this comment.
Just FYI where my concern comes from:
It is also possible to specify which locales should be included in the executable and which should be the default. For example, to switch the default locale to Swiss German and also include French and English, use the following options:
native-image -Duser.country=CH -Duser.language=de -H:IncludeLocales=fr,en
The locales are specified using language tags. You can include all locales via -H:+IncludeAllLocales, but note that it increases the size of the resulting executable.
https://www.graalvm.org/latest/reference-manual/native-image/dynamic-features/Resources/#locales
Test Results1 135 files ± 0 1 135 suites ±0 1h 27m 36s ⏱️ + 3m 47s Results for commit 0788738. ± Comparison against base commit 5feb94b. This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
flow-server/src/test/java/com/vaadin/flow/server/menu/MenuRegistryTest.java
Outdated
Show resolved
Hide resolved
With collect* methods.
flow-server/src/test/java/com/vaadin/flow/server/menu/MenuRegistryTest.java
Outdated
Show resolved
Hide resolved
|
|
This ticket/PR has been released with Vaadin 24.5.0. |
fixes: #19905