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: reorder unhandled imports in CSS bundler #19021

Merged
merged 8 commits into from
Apr 2, 2024

Conversation

sissbruecker
Copy link
Contributor

Currently CssBundler just replaces @imports with inlined file contents and returns the result. That can be problematic if an unprocessed import, for example to an external URL, comes after an inlined import. Now the unprocessed import comes after a regular CSS rule, which is invalid and ignored by the browser:

/* inlined import content */
body { background: blue; }
/* unprocessed import */
@import url('https://cdn.jsdelivr.net/fontsource/css/aclonica@latest/index.css');

This change reorders unhandled imports so that they are at the top of the file:

@import url('https://cdn.jsdelivr.net/fontsource/css/aclonica@latest/index.css');
body { background: blue; }

Copy link

github-actions bot commented Mar 22, 2024

Test Results

1 091 files  ±0  1 091 suites  ±0   1h 25m 26s ⏱️ +31s
6 937 tests +1  6 888 ✅ +1  49 💤 ±0  0 ❌ ±0 
7 285 runs  +6  7 224 ✅ +6  61 💤 ±0  0 ❌ ±0 

Results for commit 06ea4c6. ± Comparison against base commit b83dfff.

♻️ This comment has been updated with latest results.

@mcollovati mcollovati added the Contribution PRs coming from the community or external to the team label Mar 22, 2024
Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

@sissbruecker do you mind fixing the test module?

@mcollovati
Copy link
Collaborator

Doing some additional tests, I found that there is another use case that we might consider: a CSS URL with query string.
For example, if I add @import url('https://cdn.jsdelivr.net/fontsource/css/aclonica@latest/index.css?someCacheKillerParam'); to the unhandledImportsAreMovedToTop test, after processing it is put at the bottom.

@sissbruecker
Copy link
Contributor Author

Doing some additional tests, I found that there is another use case that we might consider: a CSS URL with query string.
For example, if I add @import url('https://cdn.jsdelivr.net/fontsource/css/aclonica@latest/index.css?someCacheKillerParam'); to the unhandledImportsAreMovedToTop test, after processing it is put at the bottom.

@mcollovati Changed it so that it handles query strings. Now it basically moves all unhandled imports to the top.

mcollovati
mcollovati previously approved these changes Mar 27, 2024
Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

LGTM, but please look at the comment on sanitizeUrl() method and see if it makes sense to you.

There might still be some corner cases where a stylesheet that cannot be inlined is supposed to overwrite rules from a local theme stylesheet, so changing the order might break styling.
But let's merge this fix, and then we will see whether the above is a real use case or not.

@mcollovati mcollovati enabled auto-merge (squash) April 2, 2024 12:52
@vaadin-bot vaadin-bot added +0.0.1 and removed +1.0.0 labels Apr 2, 2024
Copy link

sonarcloud bot commented Apr 2, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@mcollovati mcollovati merged commit c263d4f into main Apr 2, 2024
25 of 26 checks passed
@mcollovati mcollovati deleted the fix/reorder-unhandled-imports branch April 2, 2024 13:06
vaadin-bot pushed a commit that referenced this pull request Apr 2, 2024
Currently, CssBundler just replaces @imports with inlined file contents and returns the result. That can be problematic if an unprocessed import, for example to an external URL, comes after an inlined import. Now the unprocessed import comes after a regular CSS rule, which is invalid and ignored by the browser.
vaadin-bot added a commit that referenced this pull request Apr 2, 2024
Currently, CssBundler just replaces @imports with inlined file contents and returns the result. That can be problematic if an unprocessed import, for example to an external URL, comes after an inlined import. Now the unprocessed import comes after a regular CSS rule, which is invalid and ignored by the browser.

Co-authored-by: Sascha Ißbrücker <sissbruecker@vaadin.com>
@vaadin-bot
Copy link
Collaborator

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

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

Successfully merging this pull request may close these issues.

3 participants