Prevent removing CSS preloads during bundling (#1326)

* Prevent removing nodes, becasue styles preloading was detected earlier

* Add separate deduping for preloads and cover it with tests.

* Create quiet-horses-turn.md

* Test merging preload tags

.changeset/quiet-horses-turn.md

@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+During CSS bundling separate processing of `rel="preload"` from normal loading stylesheets, to preserve preloads, and source element attributes like `media`.

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

@@ -115,23 +115,40 @@ export async function bundleCSS({
       if (buildState[id].contentType !== 'text/html') return;
 
       const $ = cheerio.load(buildState[id].contents);
-      const pageCSS = new Set<string>(); // keep track of page-specific CSS so we remove dupes
+      const stylesheets = new Set<string>(); // keep track of page-specific CSS so we remove dupes
+      const preloads = new Set<string>(); // list of stylesheets preloads, to remove dupes
+
       $('link[href]').each((i, el) => {
         const srcPath = getSrcPath(id, { astroConfig });
         const oldHref = getDistPath($(el).attr('href') || '', { astroConfig, srcPath }); // note: this may be a relative URL; transform to absolute to find a buildOutput match
         const newHref = cssMap.get(oldHref);
-        if (newHref) {
-          // note: link[href] will select too much, however, remote CSS and non-CSS link tags won’t be in cssMap
-          if (pageCSS.has(newHref)) {
-            $(el).remove(); // this is a dupe; remove
-          } else {
-            $(el).attr('href', cssHashes.get(newHref) || ''); // new CSS; update href (important! use cssHashes, not cssMap)
-            pageCSS.add(newHref);
-          }
-          // bonus: add [rel] and [type]. not necessary, but why not?
-          $(el).attr('rel', 'stylesheet');
-          $(el).attr('type', 'text/css');
+
+        if (!newHref) {
+          return
         }
+
+        if (el.attribs?.rel === 'preload') {
+          if (preloads.has(newHref)) {
+            $(el).remove();
+          } else {
+            $(el).attr("href", cssHashes.get(newHref) || "");
+            preloads.add(newHref);
+          }
+          return
+        }
+
+        if (stylesheets.has(newHref)) {
+          $(el).remove(); // this is a dupe; remove
+        } else {
+          $(el).attr("href", cssHashes.get(newHref) || ""); // new CSS; update href (important! use cssHashes, not cssMap)
+
+          // bonus: add [rel] and [type]. not necessary, but why not?
+          $(el).attr("rel", "stylesheet");
+          $(el).attr("type", "text/css");
+
+          stylesheets.add(newHref);
+        }
+
       });
       (buildState[id] as any).contents = $.html(); // save updated HTML in global buildState
     })

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

@@ -13,6 +13,8 @@ const EXPECTED_CSS = {
   '/index.html': ['/_astro/common-', '/_astro/index-'], // don’t match hashes, which change based on content
   '/one/index.html': ['/_astro/common-', '/_astro/one/index-'],
   '/two/index.html': ['/_astro/common-', '/_astro/two/index-'],
+  '/preload/index.html': ['/_astro/common-', '/_astro/preload/index-'],
+  '/preload-merge/index.html': ['/_astro/preload-merge/index-']
 };
 const UNEXPECTED_CSS = ['/_astro/components/nav.css', '../css/typography.css', '../css/colors.css', '../css/page-index.css', '../css/page-one.css', '../css/page-two.css'];
 
@@ -28,40 +30,54 @@ CSSBundling('Bundles CSS', async (context) => {
 
     // test 1: assert new bundled CSS is present
     for (const href of css) {
-      const link = $(`link[href^="${href}"]`);
-      assert.equal(link.length, 1);
+      const link = $(`link[rel="stylesheet"][href^="${href}"]`);
+      assert.equal(link.length, 1, 'New bundled CSS is not present');
       builtCSS.add(link.attr('href'));
     }
 
     // test 2: assert old CSS was removed
     for (const href of UNEXPECTED_CSS) {
-      const link = $(`link[href="${href}"]`);
-      assert.equal(link.length, 0);
+      const link = $(`link[rel="stylesheet"][href="${href}"]`);
+      assert.equal(link.length, 0, 'Old CSS was not removed');
+    }
+
+    // test 3: preload tags was not removed and attributes was preserved
+    if (filepath === '/preload/index.html') {
+      const stylesheet = $('link[rel="stylesheet"][href^="/_astro/preload/index-"]');
+      const preload = $('link[rel="preload"][href^="/_astro/preload/index-"]');
+      assert.equal(stylesheet[0].attribs.media, 'print', 'Attribute was not preserved');
+      assert.equal(preload.length, 1, 'Preload tag was removed');
+    }
+
+    // test 4: preload tags was not removed and attributes was preserved
+    if (filepath === '/preload-merge/index.html') {
+      const preload = $('link[rel="preload"]');
+      assert.equal(preload.length, 1, 'Preload tag was not merged or was removed completly');
     }
   }
 
-  // test 3: assert all bundled CSS was built and contains CSS
+  // test 5: assert all bundled CSS was built and contains CSS
   for (const url of builtCSS.keys()) {
     const css = await context.readFile(url);
     assert.ok(css, true);
   }
 
-  // test 4: assert ordering is preserved (typography.css before colors.css)
+  // test 6: 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);
 
-  // test 5: assert multiple style blocks were bundled (Nav.astro includes 2 scoped style blocks)
+  // test 7: assert multiple style blocks were bundled (Nav.astro includes 2 scoped style blocks)
   const scopedNavStyles = [...bundledContents.matchAll('.nav.astro-')];
   assert.is(scopedNavStyles.length, 2);
 
-  // test 6: assert <style global> was not scoped (in Nav.astro)
+  // test 8: assert <style global> was not scoped (in Nav.astro)
   const globalStyles = [...bundledContents.matchAll('html{')];
   assert.is(globalStyles.length, 1);
 
-  // test 7: assert keyframes are only scoped for non-global styles (from Nav.astro)
+  // test 9: assert keyframes are only scoped for non-global styles (from Nav.astro)
   const scopedKeyframes = [...bundledContents.matchAll('nav-scoped-fade-astro')];
   const globalKeyframes = [...bundledContents.matchAll('nav-global-fade{')];
   assert.ok(scopedKeyframes.length > 0);

packages/astro/test/fixtures/astro-css-bundling/src/css/page-preload-merge-2.css

@@ -0,0 +1,3 @@
+.page__preload-merge-2 {
+  background: blanchedalmond;
+}

packages/astro/test/fixtures/astro-css-bundling/src/css/page-preload-merge.css

@@ -0,0 +1,3 @@
+.page__preload-merge {
+  background: aquamarine;
+}

packages/astro/test/fixtures/astro-css-bundling/src/css/page-preload.css

@@ -0,0 +1,3 @@
+.page__preload {
+  background: wheat;
+}

packages/astro/test/fixtures/astro-css-bundling/src/pages/preload-merge.astro

@@ -0,0 +1,17 @@
+---
+import Nav from '../components/Nav.astro';
+---
+
+<html>
+  <head>
+    <link rel="preload" as="style" href="../css/page-preload-merge.css" />
+    <link rel="preload" as="style" href="../css/page-preload-merge-2.css" />
+
+    <link rel="stylesheet" href="../css/page-preload-merge.css" />
+    <link rel="stylesheet" href="../css/page-preload-merge-2.css" />
+  </head>
+  <body>
+    <Nav />
+    <h1>Preload merge page</h1>
+  </body>
+</html>

packages/astro/test/fixtures/astro-css-bundling/src/pages/preload.astro

@@ -0,0 +1,14 @@
+---
+import Nav from '../components/Nav.astro';
+---
+
+<html>
+  <head>
+    <link rel="preload" href="../css/page-preload.css" />
+    <link rel="stylesheet" href="../css/page-preload.css" media="print" onload="this.media='all'" />
+  </head>
+  <body>
+    <Nav />
+    <h1>Preload page</h1>
+  </body>
+</html>