hilla icon indicating copy to clipboard operation
hilla copied to clipboard

Fix Hilla stats to detect Lit vs. React usage based on deployment configuration

Open taefi opened this issue 1 year ago • 6 comments

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

taefi avatar Feb 27 '24 13:02 taefi

based on DeplymentConfiguration's method called isReactEnabled.

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.

Legioth avatar Feb 27 '24 14:02 Legioth

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.

taefi avatar Feb 27 '24 14:02 taefi

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?

Legioth avatar Feb 28 '24 07:02 Legioth

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?

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 isLitEnabled and isReactEnabled
  • One method returning an enum instance to indicate either of the four possible states: NONE, LIT, REACT, LIT_REACT

taefi avatar Feb 28 '24 07:02 taefi

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

platosha avatar Mar 13 '24 12:03 platosha

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.

platosha avatar Mar 13 '24 12:03 platosha

This ticket/PR has been released with Hilla 24.4.0.beta4 and is also targeting the upcoming stable 24.4.0 version.

vaadin-bot avatar May 23 '24 09:05 vaadin-bot

This ticket/PR has been released with Hilla 24.5.0.alpha1 and is also targeting the upcoming stable 24.5.0 version.

vaadin-bot avatar May 31 '24 09:05 vaadin-bot