-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
fix: css injection order with dynamic imports (#3924) #21079
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
base: main
Are you sure you want to change the base?
Conversation
Additional Technical DetailsWhy This Fix WorksThe previous implementation used
This fix resolves that by:
Example: Diamond DependencyBefore Fix:
After Fix:
Performance Impact
Test EvidenceAll CSS test suites passing: |
Fixes AppliedI've addressed the feedback from the automated checks: 1. Fixed PR TitleChanged from: To:
2. Removed Duplicate JSDoc Comment
Commit: 35184c6 Both issues should now be resolved! |
Performance Optimization AppliedThanks to the excellent review from graphite-app bot! Fixed an inefficiency in the pending CSS queue: ProblemWhen CSS was queued for pending dependencies, we were creating a style element that was never used: // Old code - wasteful
if (!depsReady) {
style = document.createElement('style') // Created but never inserted
style.setAttribute('type', 'text/css')
style.setAttribute('data-vite-dev-id', id)
style.textContent = content
pendingCSS.set(id, { css, deps, element: style }) // Stored but discarded
return
}When SolutionDon't create the style element when queuing - just store the data: // New code - efficient
if (!depsReady) {
pendingCSS.set(id, { css: content, deps }) // Just data, no DOM element
return
}The element is now only created once, when dependencies are actually ready and the CSS is inserted. Benefits
Commit: 84f1e2d |
|
I wonder if you could try out the test cases in #9278, to see if they are addressed here. |
|
@IanVS Good idea! I'll create a test case based on that scenario - two async chunks that share a common dependency, both importing the same global CSS before their own module CSS. Let me add this test case now to verify our fix handles it correctly. |
|
@IanVS I've added a comprehensive test case for the scenario from issue #9278 in commit 53e18e3. Test Case StructureThe test validates the shared dependency scenario where:
What the Test ValidatesThe test ensures that module CSS correctly takes precedence over global CSS in the cascade order: test('async css order with shared dependency and global CSS', async () => {
const blueEl = page.locator('text=async blue').first()
const blackEl = page.locator('text=async black').first()
// Module CSS should win over global CSS
await expect.poll(() => getColor(blueEl)).toBe('blue')
await expect.poll(() => getColor(blackEl)).toBe('black')
})Even though both elements have the All tests passing (945 total) |
Fixes issue where CSS from dynamic imports loaded in wrong order in dev mode, causing cascade issues and inconsistency with build mode. Problem: - CSS from dynamic imports inserted in arbitrary order (race conditions) - Diamond dependencies loaded incorrectly - Parallel dynamic imports failed - Dev mode behavior didn't match build mode Solution: Server-side (css.ts): - Added getCssDependencies() to calculate CSS deps via module graph traversal - Handles transitive dependencies recursively - Returns ordered list of CSS dependencies Client-side (client.ts): - Modified updateStyle() to accept dependency array - Inserts CSS after dependencies to respect cascade order - Uses appendChild fallback for proper override behavior - Maintains arrival-order tracking with setTimeout reset - Added processPendingCSS() to handle transitive dependencies Test Results: - Before: 133/134 passing (99.3%) - After: 134/134 passing (100%) ✅ - Added diamond dependency test case All test suites passing, no regressions.
- Remove duplicate JSDoc block for processPendingCSS function - Fix trailing spaces in JSDoc comments
- Don't create style element when queueing CSS for pending dependencies - Element will be created when dependencies are ready and CSS is actually inserted - Removes unnecessary DOM element creation and garbage collection - Fixes Graphite bot review feedback
…js#9278) This test validates the scenario from issue vitejs#9278 where two async chunks (blue.js and black.js) share a common dependency (make-text.js), and both import global CSS (hotpink.css) before their module CSS files. The test ensures that module CSS correctly takes precedence over global CSS in the cascade order, matching the behavior of build mode. Addresses feedback from @IanVS
53e18e3 to
2711e69
Compare
Description
Fixes #3924 - CSS injection order with dynamic imports
Problem
In dev mode, CSS from dynamic imports was being inserted in the wrong order, causing:
Impact: Styles would override incorrectly, dev mode behavior didn't match production
Solution
Implemented comprehensive dependency-aware CSS insertion system:
Server-side (
packages/vite/src/node/plugins/css.ts):getCssDependencies()function that traverses the module graphClient-side (
packages/vite/src/client/client.ts):updateStyle()to acceptdepsparameter (dependency array)appendChildfallback to properly override user stylesprocessPendingCSS()to handle transitive dependency chainsremoveStyle()Algorithm
Server calculates dependencies:
Client inserts with dependencies:
Test Results
Before: 133/134 tests passing (99.3%)
After: 134/134 tests passing (100%) ✅
Previously Failing Test:
Added Test Coverage:
Test Cases Verified
✅ Diamond dependencies (A→B, A→C, B→D, C→D)
✅ Parallel dynamic imports
✅ CSS with JS wrappers
✅ Transitive dependencies (3+ levels deep)
✅ Direct dynamic CSS imports
✅ Manual DOM manipulation with style tags
✅ HMR updates (preserve position)
Breaking Changes
None - this is a bug fix that aligns dev mode with build mode behavior.
Checklist
Related Issues
Closes #3924