draft: add/move hilla dependencies #4953
Conversation
vaadin-core-internal/pom.xml
Outdated
| @@ -209,6 +209,11 @@ | |||
| <artifactId>vaadin-menu-bar-flow</artifactId> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>com.vaadin.hilla</groupId> | |||
| <artifactId>hilla</artifactId> | |||
There was a problem hiding this comment.
Would it be better to add hilla to vaadin until at least we don't have a split for core and all react components?
There was a problem hiding this comment.
Also, the idea to add hilla to vaadin-spring-boot-starter looks not that bad to me now. As Jakarta EE and Quarkus users can just not add vaadin-spring-boot-starter to their pom.xml and they won't have Spring and Hilla then.
There was a problem hiding this comment.
This would also make it pretty easy to exclude hilla by not using vaadin-spring-boot-starter because it is only a wrapper for vaadin-spring, vaadin-core and hilla. So the flow developer only has to add vaadin-spring and vaadin-core if they don't wanna deal with exclusions.
There was a problem hiding this comment.
This would also give easy option to divide it in two platform modules like vaadin-spring-boot-starter and vaadin-hilla-spring-boot-starter.
There was a problem hiding this comment.
The idea is to use one dependency vaadin-spring-boot-starter and avoid "hilla" artifacts, so having a second one with the hilla in the middle is approx. the same as have old hilla-spring-boot-starter.
vaadin-core-internal/pom.xml
Outdated
| @@ -209,6 +209,11 @@ | |||
| <artifactId>vaadin-menu-bar-flow</artifactId> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>com.vaadin.hilla</groupId> | |||
| <artifactId>hilla</artifactId> | |||
There was a problem hiding this comment.
Also I think we have to make changes in hilla artifact, so here are some possible actions:
pom.xmlcan be retained as is inhilla.@NpmPackageannotations should be removed fromhilla.hilla-reactcan be just removed from Hilla as it has the same pom.xml ashilla.hillathen is added to Platform.
5. Flow adds these three react packages (Auto Crud etc.) ifreactRouterEnabled=trueotherwise it adds one lit form package (inNodeUpdaterclass).
UPD: 5. Flow adds these three react packages (Auto Crud etc.) if reactRouterEnabled=true and Hilla is used, otherwise, if reactRouterEnabled=false and Hilla is used, it adds one lit form package. Else, if pure Flow app, it doesn't add these deps. Can be done in NodeUpdater class.
There was a problem hiding this comment.
Flow adds these three react packages (Auto Crud etc.) if reactRouterEnabled=true otherwise it adds one lit form package (in NodeUpdater class).
I would vote against this; or add "if hilla is on the class path" - otherwise flow only apps gets react packages thanks to reactRouterEnabled=true as default
There was a problem hiding this comment.
or add "if hilla is on the class path"
Yes, agree, that's a good point.
I proposed to remove npm package annotations and add packages imperatively in NodeUpdater, so that to relief developers from excluding artifacts by themselves.
There was a problem hiding this comment.
Updated the wording for 5.
|
Build fails due to |
From vaadin-core-internal.
| <!-- workaround,should be removed after this module has been added to hilla-bom --> | ||
| <dependency> | ||
| <groupId>com.vaadin.hilla</groupId> | ||
| <artifactId>hilla-dev-mode</artifactId> |
There was a problem hiding this comment.
Maybe I missed something important, but how with this structure one can exclude all development related dependencies in hybrid app ? I expected only one step as previously - excluding vaadin-dev in pom.xml.
See my comment here.
There was a problem hiding this comment.
this is only a bom, which doesn't bring in any dependency directly, as it is only under dependencyManagement.
also for this case, it is a workaround as hilla-dev-mode is not in hilla-bom yet
There was a problem hiding this comment.
Yep, agree, maybe this isn't a better place to ask this.
There was a problem hiding this comment.
However, not clear to me how Hilla dev things should be excluded in prod mode, and should they even be excluded?
related to #4934 #4946