From 27ac6a03a1b58da836190922304de5645854b49b Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Tue, 23 Aug 2022 13:26:49 -0400 Subject: [PATCH] Use CSS depth to sort CSS in production (#4446) * Use CSS depth to sort CSS in production * Adds a changeset --- .changeset/nine-pants-leave.md | 9 ++ packages/astro/src/core/build/generate.ts | 6 +- packages/astro/src/core/build/graph.ts | 15 +-- packages/astro/src/core/build/internal.ts | 21 ++++ packages/astro/src/core/build/page-data.ts | 4 +- packages/astro/src/core/build/types.ts | 2 +- .../src/core/build/vite-plugin-analyzer.ts | 4 +- .../astro/src/core/build/vite-plugin-css.ts | 30 +++-- .../astro/src/core/build/vite-plugin-ssr.ts | 4 +- packages/astro/test/css-order.test.js | 112 ++++++++++++------ .../fixtures/css-order/src/pages/two.astro | 1 + 11 files changed, 146 insertions(+), 62 deletions(-) create mode 100644 .changeset/nine-pants-leave.md diff --git a/.changeset/nine-pants-leave.md b/.changeset/nine-pants-leave.md new file mode 100644 index 000000000..75b180263 --- /dev/null +++ b/.changeset/nine-pants-leave.md @@ -0,0 +1,9 @@ +--- +'astro': patch +--- + +Deterministic CSS ordering + +This makes our CSS link order deterministic. It uses CSS depth; that is how deeply a module import the CSS comes from, in order to determine which CSS is page-level vs. component-level CSS. + +This is intended to match dev ordering where, because we do not bundle, the page-level CSS always comes after component-level. diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index 68da189fa..5a32122a7 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -28,7 +28,7 @@ import { createRequest } from '../request.js'; import { matchRoute } from '../routing/match.js'; import { getOutputFilename } from '../util.js'; import { getOutFile, getOutFolder } from './common.js'; -import { eachPageData, getPageDataByComponent } from './internal.js'; +import { eachPageData, getPageDataByComponent, sortedCSS } from './internal.js'; import type { PageBuildData, SingleFileBuiltModule, StaticBuildOptions } from './types'; import { getTimeStat } from './util.js'; @@ -124,7 +124,7 @@ async function generatePage( const renderers = ssrEntry.renderers; const pageInfo = getPageDataByComponent(internals, pageData.route.component); - const linkIds: string[] = Array.from(pageInfo?.css ?? []); + const linkIds: string[] = sortedCSS(pageData); const scripts = pageInfo?.hoistedScript ?? null; const pageModule = ssrEntry.pageMap.get(pageData.component); @@ -264,7 +264,7 @@ async function generatePath( astroConfig.base !== '/' ? joinPaths(astroConfig.site?.toString() || 'http://localhost/', astroConfig.base) : astroConfig.site; - const links = createLinkStylesheetElementSet(linkIds.reverse(), site); + const links = createLinkStylesheetElementSet(linkIds, site); const scripts = createModuleScriptsSet(hoistedScripts ? [hoistedScripts] : [], site); if (astroConfig._ctx.scripts.some((script) => script.stage === 'page')) { diff --git a/packages/astro/src/core/build/graph.ts b/packages/astro/src/core/build/graph.ts index e112845ab..beef79563 100644 --- a/packages/astro/src/core/build/graph.ts +++ b/packages/astro/src/core/build/graph.ts @@ -5,19 +5,20 @@ import { resolvedPagesVirtualModuleId } from '../app/index.js'; export function* walkParentInfos( id: string, ctx: { getModuleInfo: GetModuleInfo }, + depth = 0, seen = new Set() -): Generator { +): Generator<[ModuleInfo, number], void, unknown> { seen.add(id); const info = ctx.getModuleInfo(id); if (info) { - yield info; + yield [info, depth]; } const importers = (info?.importers || []).concat(info?.dynamicImporters || []); for (const imp of importers) { if (seen.has(imp)) { continue; } - yield* walkParentInfos(imp, ctx, seen); + yield* walkParentInfos(imp, ctx, ++depth, seen); } } @@ -26,10 +27,10 @@ export function* walkParentInfos( export function* getTopLevelPages( id: string, ctx: { getModuleInfo: GetModuleInfo } -): Generator { - for (const info of walkParentInfos(id, ctx)) { - if (info?.importers[0] === resolvedPagesVirtualModuleId) { - yield info; +): Generator<[ModuleInfo, number], void, unknown> { + for (const res of walkParentInfos(id, ctx)) { + if (res[0]?.importers[0] === resolvedPagesVirtualModuleId) { + yield res; } } } diff --git a/packages/astro/src/core/build/internal.ts b/packages/astro/src/core/build/internal.ts index 2926e8270..d7db27069 100644 --- a/packages/astro/src/core/build/internal.ts +++ b/packages/astro/src/core/build/internal.ts @@ -181,3 +181,24 @@ export function hasPageDataByViteID(internals: BuildInternals, viteid: ViteID): export function* eachPageData(internals: BuildInternals) { yield* internals.pagesByComponent.values(); } + +/** + * Sort a page's CSS by depth. A higher depth means that the CSS comes from shared subcomponents. + * A lower depth means it comes directly from the top-level page. + * The return of this function is an array of CSS paths, with shared CSS on top + * and page-level CSS on bottom. + */ +export function sortedCSS(pageData: PageBuildData) { + return Array.from(pageData.css).sort((a, b) => { + let depthA = a[1].depth, + depthB = b[1].depth; + + if(depthA === -1) { + return -1; + } else if(depthB === -1) { + return 1; + } else { + return depthA > depthB ? -1 : 1; + } + }).map(([id]) => id); +} diff --git a/packages/astro/src/core/build/page-data.ts b/packages/astro/src/core/build/page-data.ts index 5de8804c4..509d1ae20 100644 --- a/packages/astro/src/core/build/page-data.ts +++ b/packages/astro/src/core/build/page-data.ts @@ -53,7 +53,7 @@ export async function collectPagesData( component: route.component, route, moduleSpecifier: '', - css: new Set(), + css: new Map(), hoistedScript: undefined, }; @@ -74,7 +74,7 @@ export async function collectPagesData( component: route.component, route, moduleSpecifier: '', - css: new Set(), + css: new Map(), hoistedScript: undefined, }; } diff --git a/packages/astro/src/core/build/types.ts b/packages/astro/src/core/build/types.ts index 941ec35f6..90da4b0ef 100644 --- a/packages/astro/src/core/build/types.ts +++ b/packages/astro/src/core/build/types.ts @@ -18,7 +18,7 @@ export interface PageBuildData { component: ComponentPath; route: RouteData; moduleSpecifier: string; - css: Set; + css: Map; hoistedScript: { type: 'inline' | 'external'; value: string } | undefined; } export type AllPagesData = Record; diff --git a/packages/astro/src/core/build/vite-plugin-analyzer.ts b/packages/astro/src/core/build/vite-plugin-analyzer.ts index a859bcd7c..e4f64b181 100644 --- a/packages/astro/src/core/build/vite-plugin-analyzer.ts +++ b/packages/astro/src/core/build/vite-plugin-analyzer.ts @@ -22,7 +22,7 @@ export function vitePluginAnalyzer(internals: BuildInternals): VitePlugin { } if (hoistedScripts.size) { - for (const pageInfo of getTopLevelPages(from, this)) { + for (const [pageInfo] of getTopLevelPages(from, this)) { const pageId = pageInfo.id; for (const hid of hoistedScripts) { if (pageScripts.has(pageId)) { @@ -98,7 +98,7 @@ export function vitePluginAnalyzer(internals: BuildInternals): VitePlugin { clientOnlys.push(cid); } - for (const pageInfo of getTopLevelPages(id, this)) { + for (const [pageInfo] of getTopLevelPages(id, this)) { const pageData = getPageDataByViteID(internals, pageInfo.id); if (!pageData) continue; diff --git a/packages/astro/src/core/build/vite-plugin-css.ts b/packages/astro/src/core/build/vite-plugin-css.ts index 8b2a31005..1788d9ee8 100644 --- a/packages/astro/src/core/build/vite-plugin-css.ts +++ b/packages/astro/src/core/build/vite-plugin-css.ts @@ -42,8 +42,8 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] } function createNameForParentPages(id: string, ctx: { getModuleInfo: GetModuleInfo }): string { - const parents = Array.from(getTopLevelPages(id, ctx)).sort(); - const proposedName = parents.map((page) => nameifyPage(page.id)).join('-'); + const parents = Array.from(getTopLevelPages(id, ctx)); + const proposedName = parents.map(([page]) => nameifyPage(page.id)).sort().join('-'); // We don't want absurdedly long chunk names, so if this is too long create a hashed version instead. if (proposedName.length <= MAX_NAME_LENGTH) { @@ -51,7 +51,7 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] } const hash = crypto.createHash('sha256'); - for (const page of parents) { + for (const [page] of parents) { hash.update(page.id, 'utf-8'); } return hash.digest('hex').slice(0, 8); @@ -61,7 +61,7 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] id: string, ctx: { getModuleInfo: GetModuleInfo } ): Generator { - for (const info of walkParentInfos(id, ctx)) { + for (const [info] of walkParentInfos(id, ctx)) { yield* getPageDatasByClientOnlyID(internals, info.id); } } @@ -107,6 +107,10 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] // Chunks that have the viteMetadata.importedCss are CSS chunks if (meta.importedCss.size) { + //console.log(meta.importedCss, c.fileName); + + debugger; + // 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. @@ -114,7 +118,7 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] for (const [id] of Object.entries(c.modules)) { for (const pageData of getParentClientOnlys(id, this)) { for (const importedCssImport of meta.importedCss) { - pageData.css.add(importedCssImport); + pageData.css.set(importedCssImport, { depth: -1 }); } } } @@ -122,11 +126,23 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] // For this CSS chunk, walk parents until you find a page. Add the CSS to that page. for (const [id] of Object.entries(c.modules)) { - for (const pageInfo of getTopLevelPages(id, this)) { + debugger; + for (const [pageInfo, depth] of getTopLevelPages(id, this)) { const pageViteID = pageInfo.id; const pageData = getPageDataByViteID(internals, pageViteID); + for (const importedCssImport of meta.importedCss) { - pageData?.css.add(importedCssImport); + // CSS is prioritized based on depth. Shared CSS has a higher depth due to being imported by multiple pages. + // Depth info is used when sorting the links on the page. + if(pageData?.css.has(importedCssImport)) { + // eslint-disable-next-line + const cssInfo = pageData?.css.get(importedCssImport)!; + if(depth < cssInfo.depth) { + cssInfo.depth = depth; + } + } else { + pageData?.css.set(importedCssImport, { depth }); + } } } } diff --git a/packages/astro/src/core/build/vite-plugin-ssr.ts b/packages/astro/src/core/build/vite-plugin-ssr.ts index 7e373d0ea..71a58bfe4 100644 --- a/packages/astro/src/core/build/vite-plugin-ssr.ts +++ b/packages/astro/src/core/build/vite-plugin-ssr.ts @@ -12,7 +12,7 @@ import { BEFORE_HYDRATION_SCRIPT_ID, PAGE_SCRIPT_ID } from '../../vite-plugin-sc import { pagesVirtualModuleId } from '../app/index.js'; import { serializeRouteData } from '../routing/index.js'; import { addRollupInput } from './add-rollup-input.js'; -import { eachPageData } from './internal.js'; +import { eachPageData, sortedCSS } from './internal.js'; export const virtualModuleId = '@astrojs-ssr-virtual-entry'; const resolvedVirtualModuleId = '\0' + virtualModuleId; @@ -139,7 +139,7 @@ function buildManifest( routes.push({ file: '', - links: Array.from(pageData.css).reverse(), + links: sortedCSS(pageData), scripts: [ ...scripts, ...astroConfig._ctx.scripts diff --git a/packages/astro/test/css-order.test.js b/packages/astro/test/css-order.test.js index 4477d4660..402e6b619 100644 --- a/packages/astro/test/css-order.test.js +++ b/packages/astro/test/css-order.test.js @@ -4,13 +4,6 @@ import { loadFixture } from './test-utils.js'; import testAdapter from './test-adapter.js'; describe('CSS production ordering', () => { - let staticHTML, serverHTML; - let staticCSS, serverCSS; - - const commonConfig = Object.freeze({ - root: './fixtures/css-order/', - }); - function getLinks(html) { let $ = cheerio.load(html); let out = []; @@ -20,42 +13,85 @@ describe('CSS production ordering', () => { return out; } - before(async () => { - let fixture = await loadFixture({ ...commonConfig }); - await fixture.build(); - staticHTML = await fixture.readFile('/one/index.html'); - staticCSS = await Promise.all( - getLinks(staticHTML).map(async (href) => { - const css = await fixture.readFile(href); - return { href, css }; - }) - ); - }); + /** + * + * @param {import('./test-utils').Fixture} fixture + * @param {string} href + * @returns {Promise<{ href: string; css: string; }>} + */ + async function getLinkContent(fixture, href) { + const css = await fixture.readFile(href); + return { href, css }; + } - before(async () => { - let fixture = await loadFixture({ - ...commonConfig, - adapter: testAdapter(), - output: 'server', + describe('SSG and SSR parity', () => { + let staticHTML, serverHTML; + let staticCSS, serverCSS; + + const commonConfig = Object.freeze({ + root: './fixtures/css-order/', }); - await fixture.build(); + - const app = await fixture.loadTestAdapterApp(); - const request = new Request('http://example.com/one'); - const response = await app.render(request); - serverHTML = await response.text(); - serverCSS = await Promise.all( - getLinks(serverHTML).map(async (href) => { - const css = await fixture.readFile(`/client${href}`); - return { href, css }; - }) - ); + + before(async () => { + let fixture = await loadFixture({ ...commonConfig }); + await fixture.build(); + staticHTML = await fixture.readFile('/one/index.html'); + staticCSS = await Promise.all( + getLinks(staticHTML).map(href => getLinkContent(fixture, href)) + ); + }); + + before(async () => { + let fixture = await loadFixture({ + ...commonConfig, + adapter: testAdapter(), + output: 'server', + }); + await fixture.build(); + + const app = await fixture.loadTestAdapterApp(); + const request = new Request('http://example.com/one'); + const response = await app.render(request); + serverHTML = await response.text(); + serverCSS = await Promise.all( + getLinks(serverHTML).map(async (href) => { + const css = await fixture.readFile(`/client${href}`); + return { href, css }; + }) + ); + }); + + it('is in the same order for output: server and static', async () => { + const staticContent = staticCSS.map((o) => o.css); + const serverContent = serverCSS.map((o) => o.css); + + expect(staticContent).to.deep.equal(serverContent); + }); }); - it('is in the same order for output: server and static', async () => { - const staticContent = staticCSS.map((o) => o.css); - const serverContent = serverCSS.map((o) => o.css); + describe('Page vs. Shared CSS', () => { + /** @type {import('./test-utils').Fixture} */ + let fixture; + before(async () => { + fixture = await loadFixture({ + root: './fixtures/css-order/', + }); + await fixture.build(); + }); - expect(staticContent).to.deep.equal(serverContent); + it('Page level CSS is defined lower in the page', async () => { + let html = await fixture.readFile('/two/index.html'); + + const content = await Promise.all( + getLinks(html).map(href => getLinkContent(fixture, href)) + ); + + expect(content).to.have.a.lengthOf(2, 'there are 2 stylesheets'); + const [,last] = content; + + expect(last.css).to.match(/#00f/); + }); }); }); diff --git a/packages/astro/test/fixtures/css-order/src/pages/two.astro b/packages/astro/test/fixtures/css-order/src/pages/two.astro index 46d899bed..3652cb013 100644 --- a/packages/astro/test/fixtures/css-order/src/pages/two.astro +++ b/packages/astro/test/fixtures/css-order/src/pages/two.astro @@ -8,6 +8,7 @@ import CommonHead from '../components/CommonHead.astro';