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.

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

fix: filter hilla and copilot packages better #19113

Merged
merged 3 commits into from Apr 12, 2024

Conversation

tltv
Copy link
Member

@tltv tltv commented Apr 5, 2024

Filter "com/vaadin/hilla" and "com/vaadin/copilot" packages by default from the class scanner in VaadinServletContextInitializer and include only what is really needed: com.vaadin.copilot.startup and com.vaadin.hilla.startup. This speeds up mostly reload time and also startup time a bit.

Related-to: #19112

Filter "com/vaadin/hilla" and "com/vaadin/copilot" packages by default from the class scanner in VaadinServletContextInitializer and include only what is really needed: com.vaadin.copilot.CopilotIndexHtmlLoader, com.vaadin.copilot.CopilotLoader and com.vaadin.hilla.startup. This speeds up mostly reload time and also startup time a bit.

Related-to: #19112
Copy link

github-actions bot commented Apr 5, 2024

Test Results

1 099 files  ± 0  1 099 suites  ±0   1h 28m 4s ⏱️ + 4m 33s
6 944 tests ± 0  6 895 ✅ ± 0  49 💤 ±0  0 ❌ ±0 
7 273 runs   - 36  7 212 ✅  - 36  61 💤 ±0  0 ❌ ±0 

Results for commit 2b3b4ee. ± Comparison against base commit 27110fa.

♻️ This comment has been updated with latest results.

@mshabarov mshabarov requested a review from caalador April 8, 2024 11:42
caalador
caalador previously approved these changes Apr 9, 2024
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

The only thing here is how do we bring up the issue that if copilot adds some other scan required parts?
Hilla would seem to use the startup package which makes it a non issue, but copilot does not. Should we ask copilot to also use a dedicated pacakge for startup and scanning?

@tltv
Copy link
Member Author

tltv commented Apr 9, 2024

The only thing here is how do we bring up the issue that if copilot adds some other scan required parts? Hilla would seem to use the startup package which makes it a non issue, but copilot does not. Should we ask copilot to also use a dedicated pacakge for startup and scanning?

#19117 has a solution for that. It would let copilot and hilla to define what packages or classes to include/exclude.

@tltv
Copy link
Member Author

tltv commented Apr 9, 2024

Since we don't have #19117 done yet and no schedule for it, having a dedicated package like com.vaadin.copilot.startup for CopilotIndexHtmlLoader and CopilotLoader can make sense. No need to touch Flow then when adding new loaders in Copilot. Just add them in com.vaadin.copilot.startup and Flow scans them. Otherwise new loaders (or other classes that Flow should scan) would need to be added to Flow's VaadinServletContextInitializer.

But is it worth to refactor package names in Copilot? If there's no plans to add new classes that Flow should scan, then waiting that #19117 is done could make more sense. Pinging @MarcinVaadin for better insight regarding Copilot.

@Artur-
Copy link
Member

Artur- commented Apr 9, 2024

Makes sense to refactor copilot to use com.vaadin.copilot.startup rather than list classes here

@tltv
Copy link
Member Author

tltv commented Apr 9, 2024

Added com.vaadin.copilot.startup already. Refactoring copilot can happen anytime and no need to get back to change Flow. But we need a ticket for that in Copilot.

@Artur-
Copy link
Member

Artur- commented Apr 9, 2024

I can fix copilot as part of the next Flow upgrade

@Artur-
Copy link
Member

Artur- commented Apr 9, 2024

You can remove the copilot classes also already

Copy link

sonarcloud bot commented Apr 9, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@caalador caalador merged commit b3ddbdc into main Apr 12, 2024
26 of 30 checks passed
@caalador caalador deleted the fix/filter-hilla-copilot-packages branch April 12, 2024 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants