Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

draft: add/move hilla dependencies #4953

Merged
merged 6 commits into from
Jan 29, 2024
Merged

draft: add/move hilla dependencies #4953

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf994c3
draft: add/move hilla dependencies
ZheSun88 Jan 24, 2024
40abbdb
chore: move hilla to vaadin-spring-boot-starter
tltv Jan 25, 2024
4d24b05
Merge branch 'main' into move-hilla-dependency
manolo Jan 25, 2024
46be5b9
workaround: add the hilla-dev-mode to vaadin-bom
ZheSun88 Jan 26, 2024
9969c77
chore: removed hilla-dev-mode
tltv Jan 26, 2024
9f98dd7
Merge branch 'main' into move-hilla-dependency
manolo Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions scripts/generator/templates/template-vaadin-spring-bom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@

<dependencyManagement>
<dependencies>
<dependency>
<groupId>com.vaadin.hilla</groupId>
<artifactId>hilla-bom</artifactId>
<version>${hilla.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>

<dependency>
<groupId>com.vaadin</groupId>
<artifactId>vaadin</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions vaadin-core-internal/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,11 @@
<artifactId>vaadin-menu-bar-flow</artifactId>
</dependency>

<dependency>
<groupId>com.vaadin.hilla</groupId>
<artifactId>hilla</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to add hilla to vaadin until at least we don't have a split for core and all react components?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also give easy option to divide it in two platform modules like vaadin-spring-boot-starter and vaadin-hilla-spring-boot-starter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mshabarov mshabarov Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think we have to make changes in hilla artifact, so here are some possible actions:

  1. pom.xml can be retained as is in hilla.
  2. @NpmPackage annotations should be removed from hilla.
  3. hilla-react can be just removed from Hilla as it has the same pom.xml as hilla.
  4. hilla then is added to Platform.
    5. Flow adds these three react packages (Auto Crud etc.) if reactRouterEnabled=true otherwise it adds one lit form package (in NodeUpdater class).

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the wording for 5.

</dependency>

<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
Expand Down
5 changes: 5 additions & 0 deletions vaadin-dev/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@
<artifactId>vaadin-dev-bundle</artifactId>
</dependency>

<dependency>
<groupId>com.vaadin.hilla</groupId>
<artifactId>hilla-dev-mode</artifactId>
</dependency>

<dependency>
<groupId>com.vaadin</groupId>
<artifactId>copilot</artifactId>
Expand Down
3 changes: 3 additions & 0 deletions versions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@
"flow": {
"javaVersion": "24.4.0.alpha3"
},
"hilla": {
"javaVersion": "24.4.0.alpha2"
},
"flow-cdi": {
"javaVersion": "15.0.1"
},
Expand Down
Loading