Skip to content

fix: move CSS imports from @Theme to bootstrap JS file #22518

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

fix: move CSS imports from @Theme to bootstrap JS file #22518

wants to merge 13 commits into from
+162 −44

Conversation

platosha
Copy link
Contributor

Fixes #22517

@vaadin vaadin deleted a comment from github-actions bot Oct 15, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 15, 2025 •
edited
Loading

Test Results

1 256 files   -  19  1 256 suites   - 19   1h 19m 36s ⏱️ + 1m 10s
8 681 tests  - 127  8 613 ✅  - 128  66 💤  - 1  2 ❌ +2 
9 131 runs   - 128  9 052 ✅  - 130  77 💤 ±0  2 ❌ +2 

For more details on these failures, see this check.

Results for commit d8bfa48. ± Comparison against base commit d80a896.

This pull request removes 128 and adds 1 tests. Note that renamed tests count towards both. 
com.vaadin.base.devserver.AbstractDevServerRunnerTest ‑ shouldPassEncodedUrlToDevServer
com.vaadin.base.devserver.AbstractDevServerRunnerTest ‑ updateServerStartupEnvironment_preferIpv4_LocalhostIpAddressAddedToProcessEnvironment
com.vaadin.base.devserver.AbstractDevServerRunnerTest ‑ updateServerStartupEnvironment_preferIpv6_LocalhostIpAddressAddedToProcessEnvironment
com.vaadin.base.devserver.BrowserLiveReloadAccessorImplTest ‑ getLiveReload_devMode_contextHasNoReloadInstance_instanceIsCreated
com.vaadin.base.devserver.BrowserLiveReloadAccessorImplTest ‑ getLiveReload_devMode_contextHasReloadInstance_instanceIsReturned
com.vaadin.base.devserver.BrowserLiveReloadAccessorImplTest ‑ getLiveReload_liveReloadDisabled_instanceIsCreated
com.vaadin.base.devserver.BrowserLiveReloadAccessorImplTest ‑ getLiveReload_productionMode_nullIsReturned
com.vaadin.base.devserver.DebugWindowConnectionLicenseCheckTest ‑ checkLicense_invalidLicense_sendLicenseCheckFailed
com.vaadin.base.devserver.DebugWindowConnectionLicenseCheckTest ‑ checkLicense_noLicenseKeys_sendLicenseCheckFailed
com.vaadin.base.devserver.DebugWindowConnectionLicenseCheckTest ‑ checkLicense_validLicense_sendLicenseOk
…

com.vaadin.flow.server.frontend.UpdateImportsWithByteCodeScannerTest ‑ cssImportFromAppShellAndThemeWork

♻️ This comment has been updated with latest results.

caalador
caalador requested changes Oct 20, 2025
View reviewed changes
flow-server/src/main/java/com/vaadin/flow/server/frontend/scanner/FrontendDependencies.java Outdated
entryPoint.getModulesDevelopmentOnly()
.addAll(classInfo.modulesDevelopmentOnly);
if (classInfo.loadCss) {
if (classInfo.loadCss || activeTheme
Copy link
Contributor

Choose a reason for hiding this comment

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

the loadCss is always false for all theme files and activeTheme now lets through all theme files even with loadCss false. So the flag can be fully removed.

...pplication-theme/src/test/java/com/vaadin/flow/webcomponent/ApplicationThemeComponentIT.java Outdated
Assert.assertEquals(
"Project contains 4 css injections to document and all should be hashed",
4l, getCommandExecutor().executeScript(
"Project contains 2 css injections to document and all should be hashed",
Copy link
Contributor

Choose a reason for hiding this comment

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

What global css has embedded applications lost?

flow-server/src/main/java/com/vaadin/flow/server/frontend/AbstractUpdateImports.java

protected <T> List<String> merge(Map<T, List<String>> css) {
protected <T> List<String> merge(Map<T, List<String>> outputFiles) {
// Ignore app shell imports and definitions for bundle build detection,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't app shell imports also be taken into account by flow as they are not added to the general imports anymore even though they were there before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apart from unit tests, the feature that is affected here is the bundle build.

Should the bundle include theme CSS imports? I don't know, but the current implementation didn't. So I assumed that my changes at some point produced a valid failure in the bundle ITs because of including CSS imports again. Ignoring here addresses this failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mshabarov what was the css state for bunldes.
From what I see in tests and code I would expect css to be bundles and Theme css especially

@vaadin vaadin deleted a comment from github-actions bot Oct 21, 2025
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
2364 New issues
2 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@caalador caalador caalador requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

+0.0.1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Hilla applications using Lumo theme appear receive only base styles

3 participants

@platosha @caalador @vaadin-bot