Repository navigation
Add an experimental flag to include only used component styles and scripts - #18326
florian-lefebvre wants to merge 5 commits into
Conversation
Excludes the styles and scripts of Astro components that a page imports but never renders, for prerendered and on-demand rendered pages.
🦋 Changeset detectedLatest commit: 2f51e76 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will not alter performance
|
- Replay referenced assets for incremental-cache skipped pages so pruning keeps the files their restored HTML links - Derive CSS ownership from the module graph so styles shared by several components survive while any owner renders - Align the experimental flag JSDoc with the on-demand streaming behavior
Keep stylesheets that a page imports directly even when a component also imports them, expand pruned assets along the client chunk graph so CSS loaded from JS is kept, and keep every style on pages that use a server island.
The experimental.treeShakeComponents work bumped the incremental cache version, so update the assertion that still expected version 1.
Attribution ran in the client graph, where a page appears only as its script entry. The bare page path had no module info, so its stylesheet was credited to the page as a component owner, then removed at render time and pruned in static builds. Resolve page-ness through the vite id so CSS imported by a page script is always kept.
There was a problem hiding this comment.
Findings
[medium][correctness]packages/astro/src/core/build/generate.ts:457- Static pruning can delete assets that a page still links when output file names don't start withbuild.assets. Details are in the inline comment.
Merge Requirement Assessment
Status: gaps found
Exclude styles of imported-but-unrendered Astro components- Met.renderAstroComponentrecordsrenderedComponents. Build-time ownership comes fromgetChunkComponentOwnersand is passed throughStylesheetAsset.ownersintoSSRElement.owners.removeUnusedComponentStylesthen drops the unowned tags after a buffered render. The static and SSR fixtures cover this, including the shared-CSS and page-level-CSS edge cases.Split non-page components into their own chunks- Met.componentManualChunksis applied to the prerender, client and ssr environments.Static builds delete unused script and CSS files- Gap. It works with the default_astro/[name].[hash]naming, and incremental replay is handled. With custom output file names, though, assets that pages use are deleted too (see the finding).On-demand pages opt out of streaming when the flag is on- Met.handlePagespassesstreaming && !state.manifest.treeShakeComponents, and the manifest serializestreeShakeComponents.Page-level styles are always kept- Met.getCssModuleComponentOwnersreturns an empty set when a stylesheet can be reached from a page, and theshared-pageandpage-scripttests cover this.
Review Status
Verdict: changes required
Context: PR #18326 plus the PR description
Scope: PR base 3cbd72cf to head 2f51e760, every changed file
Changeset: present and covers astro (minor)
Limitations: I did not run the Rolldown chunking behavior, so I couldn't confirm it; I relied on the PR's tests for it. Docs for the new experimental flag are noted in the PR as still needed.
Validation: Static review only. I did not run any project code, tests, builds or checks.
GitHub context: collected
Git scope: PR base and head verified
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
| */ | ||
| function collectReferencedAssets(html: string, assetsDir: string, into: Set<string>): void { | ||
| const escapedDir = assetsDir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| const re = new RegExp(`${escapedDir}/[^"'?\\s>]+?\\.(?:js|css)`, 'g'); |
There was a problem hiding this comment.
[medium][correctness]: Pruning deletes linked assets when output file names don't start with build.assets
collectReferencedAssets only counts a URL as referenced when it matches ${build.assets}/…\.(js|css). componentOwnedFiles, however, holds the real Rolldown fileName of each component script chunk and CSS asset, and pruneUnusedComponentAssets deletes every one of those that isn't in the reachable set.
Users can change output names. One way is vite.build.rolldownOptions.output.assetFileNames, which the prerender build inherits through the top-level spread in vite-build-config.ts. Another is vite.environments.client.build.rolldownOptions.output.{entryFileNames,chunkFileNames,assetFileNames}, which is spread after Astro's defaults. If names are set to assets/[name].[hash][extname] or js/[name].js, no component asset matches the regex. Every component-owned file then counts as unreferenced and gets deleted, including the scripts and stylesheets of components that did render. The generated HTML ends up linking files that no longer exist.
Suggested fix: compare against the actual emitted file names rather than a build.assets prefix. For example, check the HTML for each componentOwnedFiles entry, or record referenced files from the rendered elements' href/src. Alternatively, skip pruning when a file in componentOwnedFiles falls outside build.assets. Adding a test with custom assetFileNames would catch this.
|
Does this work by splitting CSS into separate chunks per component and using propagation to wait on them? That's the only way I can see how this might work (I haven't reviewed the code deeply yet, but say the chunk splitting). |
|
To answer my own question, it doesn't need head propagation because it disables streaming instead: http://github.com/withastro/astro/pull/18326/changes#diff-f38d7b881dd4df817c6613de92d5820ac8da349cccaeac4043684f429567b8f7R60 Worth thinking about if that is the right trade-off or not, or if head propagation could/should be used. Thinking out loud, if you use head propagation here you are basically disable streaming anyways, since every deeply nested component is likely going to have styles. So probably fine going this way. I think this means this can never be the default, however. Also the chunk splitting means that too. But possibly a worthy trade-off for some. |
|
Have you considered other API ways of doing this? A boolean that toggles 2 fairly drastically different paths is not great from a maintenance perspective. What about something like a special component? ---
import Wait from 'astro:styles';
---
<Wait for={() => Math.random() > 0.5}>
<Inner>will not have styles applied unless condition is true</Inner>
</Wait>This could then use head propagation. You would only use it on subtrees that you want to prevent having styles loaded for. So the following advantages:
|
Changes
experimental.treeShakeComponents, which excludes the styles and scripts of Astro components that a page imports but never renders. This targets the CMS page-builder pattern, where a page imports every possible component and selects which one to render from data at runtime.Testing
test/experimental-tree-shake-components.test.ts, with a static fixture and an SSR fixture. It checks that only the rendered component's CSS is linked, page-level CSS is kept, and the unrendered components' script chunks are pruned.SSRMetadatamock for the new metadata fields.Docs