hilla
hilla copied to clipboard
Fix Hilla stats to detect Lit vs. React usage based on deployment configuration
Describe the bug
Hilla stats where being gathered based on the usage of hilla-react artifact, and now that the hilla-react is not there anymore, we can decide whether is it a Lit or React app based on DeplymentConfiguration's method called isReactEnabled.
Expected-behavior
- The statistics should be gathered once per application startup.
- It would be ideal if the statistics gathered based on real usages of Hilla, not just Hilla artifacts being present on classpath, as in Vaadin 24.4+, Hilla is automatically part of the platform, unless it is excluded explicitely.
Reproduction
Previous unit test for HillaStats is failing.
System Info
N/A
based on
DeplymentConfiguration's method calledisReactEnabled.
That won't be reliable in the case of application that use both Lit and React side-by-side since the current idea of how to make sure you have both @vaadin/react-components and each individual web component explicitly listed in package.json (for IDE auto complete) would be to set isReactEnabled to false and then manually add the @vaadin/react-components dependency.
That's correct, as just isReactEnabled() returning true doesn't necessarily means there is no Lit in that project. However, those projects might not be a very big share of the Vaadin-Hilla projects out there at the moment, and for them we will end up gathering wrong numbers by counting React+Lit apps as React ones.
I suspect isReactEnabled() is wrongly named. It should actually be isLitDisabled() instead.
I wonder if the logic should be such that if @vaadin/react-components is present in package.json, then we consider React to be used. If isReactEnabled() returns false, then we consider Lit to be used and if both conditions are true, then we consider both React and Lit to be used?
I suspect
isReactEnabled()is wrongly named. It should actually beisLitDisabled()instead.I wonder if the logic should be such that if
@vaadin/react-componentsis present inpackage.json, then we consider React to be used. IfisReactEnabled()returnsfalse, then we consider Lit to be used and if both conditions are true, then we consider both React and Lit to be used?
Even though, this should work now, this logic doesn't seem to be very fluent. I wonder if we should allow having @vaadin/react-components in package.json (I assume this comes by default from the platform) and enabling the user (explicitly) setting react.enabled property to false at the same time. I mean shouldn't we be able to exclude @vaadin/react-components when react.enabled is set to false?
Probably this discussion is a bit off-topic here and we should just be talking about what we need from Flow APIs, e.g. either we need
- Two independent methods such as
isLitEnabledandisReactEnabled - One method returning an enum instance to indicate either of the four possible states:
NONE,LIT,REACT,LIT_REACT
There's a bunch of detection methods added in Flow recently that could be helpful. See https://github.com/vaadin/flow/blob/main/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java
There is a unit-test HillaStatsTest. I would suggest to convert it to IT test, to verify that the feature works in combination with Flow.
This ticket/PR has been released with Hilla 24.4.0.beta4 and is also targeting the upcoming stable 24.4.0 version.
This ticket/PR has been released with Hilla 24.5.0.alpha1 and is also targeting the upcoming stable 24.5.0 version.