Bugfix: random CSS ordering (#232)

* Bugfix: random CSS ordering

Fixes #230

* Update docs

.changeset/shaggy-swans-lick.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Bugfix: CSS bundling randomizes order

docs/styling.md

@@ -132,6 +132,8 @@ All CSS is minified and bundled automatically for you in running `astro build`.
 
 We’ll be expanding our styling optimization story over time, and would love your feedback! If `astro build` generates unexpected styles, or if you can think of improvements, [please open an issue](https://github.com/snowpackjs/astro/issues).
 
+**⚠️ Ordering may not be guaranteed**. Though we try and preserve your ordering as much as possible, there are no guarantees between pages. For example, if one page loads `a.css`, `b.css`, and another loads `b.css`, there’s a chance that `b.css` may load first (as it’s a shared asset). The best course of action is to reduce your styles’ dependence on ordering as much as possible. But if specific order is required, [Sass’ @use is highly recommended][sass-use] or you must guarantee that all styles load in the same order on every page.
+
 ## 📚 Advanced Styling Architecture in Astro
 
 Many development setups give you the basics on where to put CSS for small things, but very quickly you realize the simple examples fail you as your code scales, and soon you have a mess on your hands. An natural question for any setup is “how do I architect my styles?” yet not every framework outlines its rules. While we’d like to say _“use whatever you want,”_ and leave it up to you, the reality is that not all styling approaches will work as well in Astro as others, because Astro is a blend of new ideas (**Partial Hydration** and **JS frameworks with zero runtime JS**) and old (**HTML-first**). It couldn’t hurt to try and provide a guide on how to think about styling architecture in Astro, and this section is precisely that.

packages/astro/src/build.ts

@@ -16,7 +16,7 @@ import { bundleJS, collectJSImports } from './build/bundle/js';
 import { buildCollectionPage, buildStaticPage, getPageType } from './build/page.js';
 import { generateSitemap } from './build/sitemap.js';
 import { logURLStats, collectBundleStats, mapBundleStatsToURLStats } from './build/stats.js';
-import { getDistPath, sortSet, stopTimer } from './build/util.js';
+import { getDistPath, stopTimer } from './build/util.js';
 import { debug, defaultLogDestination, error, info, warn, trapWarn } from './logger.js';
 import { createRuntime } from './runtime.js';
 
@@ -257,10 +257,7 @@ export function findDeps(html: string, { astroConfig, srcPath }: { astroConfig:
     }
   });
 
-  // sort (makes things a bit more predictable)
-  pageDeps.js = sortSet(pageDeps.js);
-  pageDeps.css = sortSet(pageDeps.css);
-  pageDeps.images = sortSet(pageDeps.images);
+  // important: preserve the scan order of deps! order matters on pages
 
   return pageDeps;
 }

packages/astro/src/build/bundle/css.ts

@@ -41,7 +41,10 @@ export async function bundleCSS({
 
   // 1. organize CSS into common or page-specific CSS
   timer.bundle = performance.now();
-  for (const [pageUrl, { css }] of Object.entries(depTree)) {
+  const sortedPages = Object.keys(depTree); // these were scanned in parallel; sort to create somewhat deterministic order
+  sortedPages.sort((a, b) => a.localeCompare(b, 'en', { numeric: true }));
+  for (const pageUrl of sortedPages) {
+    const { css } = depTree[pageUrl];
     for (const cssUrl of css.keys()) {
       if (cssMap.has(cssUrl)) {
         // scenario 1: if multiple URLs require this CSS, upgrade to common chunk
@@ -53,30 +56,26 @@ export async function bundleCSS({
     }
   }
 
-  // 2. bundle
+  // 2. bundle (note: assume cssMap keys are in specific, correct order; assume buildState[] keys are in different order each time)
   timer.bundle = performance.now();
-  await Promise.all(
-    Object.keys(buildState).map(async (id) => {
-      if (buildState[id].contentType !== 'text/css') return;
+  // note: don’t parallelize here otherwise CSS may end up in random order
+  for (const id of cssMap.keys()) {
+    const newUrl = cssMap.get(id) as string;
 
-      const newUrl = cssMap.get(id);
-      if (!newUrl) return;
+    // if new bundle, create
+    if (!buildState[newUrl]) {
+      buildState[newUrl] = {
+        srcPath: getSrcPath(id, { astroConfig }), // this isn’t accurate, but we can at least reference a file in the bundle
+        contents: '',
+        contentType: 'text/css',
+        encoding: 'utf8',
+      };
+    }
 
-      // if new bundle, create
-      if (!buildState[newUrl]) {
-        buildState[newUrl] = {
-          srcPath: getSrcPath(id, { astroConfig }), // this isn’t accurate, but we can at least reference a file in the bundle
-          contents: '',
-          contentType: 'text/css',
-          encoding: 'utf8',
-        };
-      }
-
-      // append to bundle, delete old file
-      (buildState[newUrl] as any).contents += Buffer.isBuffer(buildState[id].contents) ? buildState[id].contents.toString('utf8') : buildState[id].contents;
-      delete buildState[id];
-    })
-  );
+    // append to bundle, delete old file
+    (buildState[newUrl] as any).contents += Buffer.isBuffer(buildState[id].contents) ? buildState[id].contents.toString('utf8') : buildState[id].contents;
+    delete buildState[id];
+  }
   debug(logging, 'css', `bundled [${stopTimer(timer.bundle)}]`);
 
   // 3. minify

packages/astro/src/build/util.ts

@@ -13,11 +13,6 @@ export function canonicalURL(url: string, base?: string): URL {
   return new URL(pathname, base);
 }
 
-/** Sort a Set */
-export function sortSet(set: Set<string>): Set<string> {
-  return new Set([...set].sort((a, b) => a.localeCompare(b, 'en', { numeric: true })));
-}
-
 /** Resolve final output URL */
 export function getDistPath(specifier: string, { astroConfig, srcPath }: { astroConfig: AstroConfig; srcPath: URL }): string {
   if (specifier[0] === '/') return specifier; // assume absolute URLs are correct

packages/astro/test/astro-css-bundling.test.js

@@ -45,6 +45,13 @@ CSSBundling('Bundles CSS', async (context) => {
     const css = await context.readFile(url);
     assert.ok(css, true);
   }
+
+  // test 4: assert ordering is preserved (typography.css before colors.css)
+  const bundledLoc = [...builtCSS].find((k) => k.startsWith('/_astro/common-'));
+  const bundledContents = await context.readFile(bundledLoc);
+  const typographyIndex = bundledContents.indexOf('body{');
+  const colorsIndex = bundledContents.indexOf(':root{');
+  assert.ok(typographyIndex < colorsIndex);
 });
 
 CSSBundling.run();