From 19adcdd2c89ee1cca7e925f265a90937e62cc8a4 Mon Sep 17 00:00:00 2001 From: bluwy Date: Fri, 26 May 2023 15:43:31 +0800 Subject: [PATCH 1/6] Add failing test --- packages/astro/test/0-css.test.js | 15 +++++++++++++++ .../_components/SvelteOnlyAndSsr.svelte | 11 +++++++++++ .../src/pages/client-only-and-ssr/only.astro | 7 +++++++ .../0-css/src/pages/client-only-and-ssr/ssr.astro | 7 +++++++ 4 files changed, 40 insertions(+) create mode 100644 packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/_components/SvelteOnlyAndSsr.svelte create mode 100644 packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/only.astro create mode 100644 packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/ssr.astro diff --git a/packages/astro/test/0-css.test.js b/packages/astro/test/0-css.test.js index 1583891328e4..76bfba296f3c 100644 --- a/packages/astro/test/0-css.test.js +++ b/packages/astro/test/0-css.test.js @@ -265,6 +265,21 @@ describe('CSS', function () { new RegExp(`.svelte-scss.${scopedClass}[^{]*{font-family:ComicSansMS`) ); }); + + it('client:only and SSR in two pages, both should have styles', async () => { + const onlyHtml = await fixture.readFile('/client-only-and-ssr/only/index.html'); + const $onlyHtml = cheerio.load(onlyHtml); + const onlyHtmlCssHref = $onlyHtml('link[rel=stylesheet][href^=/_astro/]').attr('href'); + const onlyHtmlCss = await fixture.readFile(onlyHtmlCssHref.replace(/^\/?/, '/')); + + const ssrHtml = await fixture.readFile('/client-only-and-ssr/ssr/index.html'); + const $ssrHtml = cheerio.load(ssrHtml); + const ssrHtmlCssHref = $ssrHtml('link[rel=stylesheet][href^=/_astro/]').attr('href'); + const ssrHtmlCss = await fixture.readFile(ssrHtmlCssHref.replace(/^\/?/, '/')); + + expect(onlyHtmlCss).to.include('.svelte-only-and-ssr'); + expect(ssrHtmlCss).to.include('.svelte-only-and-ssr'); + }); }); describe('Vite features', () => { diff --git a/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/_components/SvelteOnlyAndSsr.svelte b/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/_components/SvelteOnlyAndSsr.svelte new file mode 100644 index 000000000000..f346f9c04d57 --- /dev/null +++ b/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/_components/SvelteOnlyAndSsr.svelte @@ -0,0 +1,11 @@ + + +
+ Svelte only and SSR +
+ + diff --git a/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/only.astro b/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/only.astro new file mode 100644 index 000000000000..b409693b5d03 --- /dev/null +++ b/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/only.astro @@ -0,0 +1,7 @@ +--- +import SvelteOnlyAndSsr from './_components/SvelteOnlyAndSsr.svelte' +--- + +
+ +
diff --git a/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/ssr.astro b/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/ssr.astro new file mode 100644 index 000000000000..35b7fbae07df --- /dev/null +++ b/packages/astro/test/fixtures/0-css/src/pages/client-only-and-ssr/ssr.astro @@ -0,0 +1,7 @@ +--- +import SvelteOnlyAndSsr from './_components/SvelteOnlyAndSsr.svelte' +--- + +
+ +
From 08f8ba2f4dcb58f5c34e2d41d072542f8c72bbec Mon Sep 17 00:00:00 2001 From: bluwy Date: Fri, 26 May 2023 15:48:36 +0800 Subject: [PATCH 2/6] Apply fix --- .../src/core/build/plugins/plugin-css.ts | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/packages/astro/src/core/build/plugins/plugin-css.ts b/packages/astro/src/core/build/plugins/plugin-css.ts index a5bdb70f112b..fd8935524eaa 100644 --- a/packages/astro/src/core/build/plugins/plugin-css.ts +++ b/packages/astro/src/core/build/plugins/plugin-css.ts @@ -64,15 +64,6 @@ function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] { const cssBuildPlugin: VitePlugin = { name: 'astro:rollup-plugin-build-css', - transform(_, id) { - // In the SSR build, styles that are bundled are tracked in `internals.cssChunkModuleIds`. - // In the client build, if we're also bundling the same style, return an empty string to - // deduplicate the final CSS output. - if (options.target === 'client' && internals.cssChunkModuleIds.has(id)) { - return ''; - } - }, - outputOptions(outputOptions) { // Skip in client builds as its module graph doesn't have reference to Astro pages // to be able to chunk based on it's related top-level pages. @@ -122,6 +113,18 @@ function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] { } } + // In the client build, we bail if the chunk is a duplicated CSS chunk tracked from + // above. We remove all the importedCss to prevent emitting the CSS asset. + if (options.target === 'client') { + if (Object.keys(chunk.modules).every((id) => internals.cssChunkModuleIds.has(id))) { + for (const importedCssImport of meta.importedCss) { + delete bundle[importedCssImport]; + meta.importedCss.delete(importedCssImport); + } + return; + } + } + // For the client build, client:only styles need to be mapped // over to their page. For this chunk, determine if it's a child of a // client:only component and if so, add its CSS to the page it belongs to. From f8b4abef5f2a2963fa390b601269d684c342cdb1 Mon Sep 17 00:00:00 2001 From: bluwy Date: Fri, 26 May 2023 16:29:39 +0800 Subject: [PATCH 3/6] Fix CSS dedupe strategy fixes "remove unused styles from client:load components" test --- packages/astro/src/core/build/internal.ts | 9 ++++++++ .../src/core/build/plugins/plugin-css.ts | 21 +++++++++++++------ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/packages/astro/src/core/build/internal.ts b/packages/astro/src/core/build/internal.ts index 18d28ef74907..c89ec856b28b 100644 --- a/packages/astro/src/core/build/internal.ts +++ b/packages/astro/src/core/build/internal.ts @@ -12,6 +12,14 @@ export interface BuildInternals { * SSR build and client build in vite-plugin-css. */ cssChunkModuleIds: Set; + /** + * Each CSS module is named with a chunk id derived from the Astro pages they + * are used in by default. It's easy to crawl this relation in the SSR build as + * the Astro pages are the entrypoint, but not for the client build as hydratable + * components are the entrypoint instead. This map is used as a cache from the SSR + * build so the client can pick up the same information and use the same chunk ids. + */ + cssModuleToChunkId: Map; // A mapping of hoisted script ids back to the exact hoisted scripts it references hoistedScriptIdToHoistedMap: Map>; @@ -93,6 +101,7 @@ export function createBuildInternals(): BuildInternals { return { cssChunkModuleIds: new Set(), + cssModuleToChunkId: new Map(), hoistedScriptIdToHoistedMap, hoistedScriptIdToPagesMap, entrySpecifierToBundleMap: new Map(), diff --git a/packages/astro/src/core/build/plugins/plugin-css.ts b/packages/astro/src/core/build/plugins/plugin-css.ts index fd8935524eaa..6b2788bfda25 100644 --- a/packages/astro/src/core/build/plugins/plugin-css.ts +++ b/packages/astro/src/core/build/plugins/plugin-css.ts @@ -65,10 +65,6 @@ function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] { name: 'astro:rollup-plugin-build-css', outputOptions(outputOptions) { - // Skip in client builds as its module graph doesn't have reference to Astro pages - // to be able to chunk based on it's related top-level pages. - if (options.target === 'client') return; - const assetFileNames = outputOptions.assetFileNames; const namingIncludesHash = assetFileNames?.toString().includes('[hash]'); const createNameForParentPages = namingIncludesHash @@ -80,16 +76,29 @@ function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] { // For CSS, create a hash of all of the pages that use it. // This causes CSS to be built into shared chunks when used by multiple pages. if (isBuildableCSSRequest(id)) { + // For client builds that has hydrated components as entrypoints, there's no way + // to crawl up and find the pages that use it. So we lookup the cache here to derive + // the same chunk id so they match up on build. + // Some modules may not exist in cache (e.g. `client:only` components), and that's okay. + // We can use Rollup's default chunk strategy instead. + if (options.target === 'client') { + return internals.cssModuleToChunkId.get(id)!; + } + for (const [pageInfo] of walkParentInfos(id, { getModuleInfo: meta.getModuleInfo, })) { if (new URL(pageInfo.id, 'file://').searchParams.has(PROPAGATED_ASSET_FLAG)) { // Split delayed assets to separate modules // so they can be injected where needed - return createNameHash(id, [id]); + const chunkId = createNameHash(id, [id]); + internals.cssModuleToChunkId.set(id, chunkId); + return chunkId; } } - return createNameForParentPages(id, meta); + const chunkId = createNameForParentPages(id, meta); + internals.cssModuleToChunkId.set(id, chunkId); + return chunkId; } }, }); From 93a9e7a581dfb0ece19b537551e60ea6dd9322da Mon Sep 17 00:00:00 2001 From: bluwy Date: Fri, 26 May 2023 16:30:51 +0800 Subject: [PATCH 4/6] Remove old CSS deduping strategy --- packages/astro/src/core/build/internal.ts | 6 ------ .../src/core/build/plugins/plugin-css.ts | 21 ------------------- 2 files changed, 27 deletions(-) diff --git a/packages/astro/src/core/build/internal.ts b/packages/astro/src/core/build/internal.ts index c89ec856b28b..d25d35f4f512 100644 --- a/packages/astro/src/core/build/internal.ts +++ b/packages/astro/src/core/build/internal.ts @@ -7,11 +7,6 @@ import { ASTRO_PAGE_EXTENSION_POST_PATTERN, ASTRO_PAGE_MODULE_ID } from './plugi import type { PageBuildData, StylesheetAsset, ViteID } from './types'; export interface BuildInternals { - /** - * The module ids of all CSS chunks, used to deduplicate CSS assets between - * SSR build and client build in vite-plugin-css. - */ - cssChunkModuleIds: Set; /** * Each CSS module is named with a chunk id derived from the Astro pages they * are used in by default. It's easy to crawl this relation in the SSR build as @@ -100,7 +95,6 @@ export function createBuildInternals(): BuildInternals { const hoistedScriptIdToPagesMap = new Map>(); return { - cssChunkModuleIds: new Set(), cssModuleToChunkId: new Map(), hoistedScriptIdToHoistedMap, hoistedScriptIdToPagesMap, diff --git a/packages/astro/src/core/build/plugins/plugin-css.ts b/packages/astro/src/core/build/plugins/plugin-css.ts index 6b2788bfda25..87dc563d0a1b 100644 --- a/packages/astro/src/core/build/plugins/plugin-css.ts +++ b/packages/astro/src/core/build/plugins/plugin-css.ts @@ -113,27 +113,6 @@ function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] { // Skip if the chunk has no CSS, we want to handle CSS chunks only if (meta.importedCss.size < 1) continue; - // In the SSR build, keep track of all CSS chunks' modules as the client build may - // duplicate them, e.g. for `client:load` components that render in SSR and client - // for hydation. - if (options.target === 'server') { - for (const id of Object.keys(chunk.modules)) { - internals.cssChunkModuleIds.add(id); - } - } - - // In the client build, we bail if the chunk is a duplicated CSS chunk tracked from - // above. We remove all the importedCss to prevent emitting the CSS asset. - if (options.target === 'client') { - if (Object.keys(chunk.modules).every((id) => internals.cssChunkModuleIds.has(id))) { - for (const importedCssImport of meta.importedCss) { - delete bundle[importedCssImport]; - meta.importedCss.delete(importedCssImport); - } - return; - } - } - // For the client build, client:only styles need to be mapped // over to their page. For this chunk, determine if it's a child of a // client:only component and if so, add its CSS to the page it belongs to. From de4f483dfbeb74817ef1ffb989ba979f8906f0d8 Mon Sep 17 00:00:00 2001 From: bluwy Date: Fri, 26 May 2023 16:40:49 +0800 Subject: [PATCH 5/6] Improve docs --- packages/astro/src/core/build/internal.ts | 4 ++-- .../astro/src/core/build/plugins/plugin-css.ts | 16 +++++++++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/astro/src/core/build/internal.ts b/packages/astro/src/core/build/internal.ts index d25d35f4f512..d1e2404d6d8a 100644 --- a/packages/astro/src/core/build/internal.ts +++ b/packages/astro/src/core/build/internal.ts @@ -14,7 +14,7 @@ export interface BuildInternals { * components are the entrypoint instead. This map is used as a cache from the SSR * build so the client can pick up the same information and use the same chunk ids. */ - cssModuleToChunkId: Map; + cssModuleToChunkIdMap: Map; // A mapping of hoisted script ids back to the exact hoisted scripts it references hoistedScriptIdToHoistedMap: Map>; @@ -95,7 +95,7 @@ export function createBuildInternals(): BuildInternals { const hoistedScriptIdToPagesMap = new Map>(); return { - cssModuleToChunkId: new Map(), + cssModuleToChunkIdMap: new Map(), hoistedScriptIdToHoistedMap, hoistedScriptIdToPagesMap, entrySpecifierToBundleMap: new Map(), diff --git a/packages/astro/src/core/build/plugins/plugin-css.ts b/packages/astro/src/core/build/plugins/plugin-css.ts index 87dc563d0a1b..1cec972239d8 100644 --- a/packages/astro/src/core/build/plugins/plugin-css.ts +++ b/packages/astro/src/core/build/plugins/plugin-css.ts @@ -77,12 +77,14 @@ function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] { // This causes CSS to be built into shared chunks when used by multiple pages. if (isBuildableCSSRequest(id)) { // For client builds that has hydrated components as entrypoints, there's no way - // to crawl up and find the pages that use it. So we lookup the cache here to derive - // the same chunk id so they match up on build. - // Some modules may not exist in cache (e.g. `client:only` components), and that's okay. - // We can use Rollup's default chunk strategy instead. + // to crawl up and find the pages that use it. So we lookup the cache during SSR + // build (that has the pages information) to derive the same chunk id so they + // match up on build, making sure both builds has the CSS deduped. + // NOTE: Components that are only used with `client:only` may not exist in the cache + // and that's okay. We can use Rollup's default chunk strategy instead as these CSS + // are outside of the SSR build scope, which no dedupe is needed. if (options.target === 'client') { - return internals.cssModuleToChunkId.get(id)!; + return internals.cssModuleToChunkIdMap.get(id)!; } for (const [pageInfo] of walkParentInfos(id, { @@ -92,12 +94,12 @@ function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] { // Split delayed assets to separate modules // so they can be injected where needed const chunkId = createNameHash(id, [id]); - internals.cssModuleToChunkId.set(id, chunkId); + internals.cssModuleToChunkIdMap.set(id, chunkId); return chunkId; } } const chunkId = createNameForParentPages(id, meta); - internals.cssModuleToChunkId.set(id, chunkId); + internals.cssModuleToChunkIdMap.set(id, chunkId); return chunkId; } }, From caeef2f219ee39d1f083fe924015a42d9bd39e40 Mon Sep 17 00:00:00 2001 From: bluwy Date: Fri, 26 May 2023 16:57:32 +0800 Subject: [PATCH 6/6] Add changeset --- .changeset/eighty-gifts-cheer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/eighty-gifts-cheer.md diff --git a/.changeset/eighty-gifts-cheer.md b/.changeset/eighty-gifts-cheer.md new file mode 100644 index 000000000000..41ca0d7f628e --- /dev/null +++ b/.changeset/eighty-gifts-cheer.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fix CSS deduping and missing chunks