From 0fa772b075e03b767fcbf7827fa511fe48a6055e Mon Sep 17 00:00:00 2001
From: Drew Powers <drew@pow.rs>
Date: Tue, 30 Mar 2021 15:06:31 -0600
Subject: [PATCH] Extract Astro styles to external stylesheets

---
 .eslintrc.cjs                   |  2 +
 .github/workflows/nodejs.yml    |  7 +++
 snowpack-plugin.cjs             |  8 ++-
 src/@types/astro.ts             |  2 +
 src/compiler/codegen.ts         | 34 ++++++++++---
 src/compiler/index.ts           | 15 +++---
 src/compiler/optimize/styles.ts | 36 ++++---------
 src/runtime.ts                  | 90 +++++++++++++++++++--------------
 test/astro-styles-ssr.test.js   | 25 ++++++---
 9 files changed, 130 insertions(+), 89 deletions(-)

diff --git a/.eslintrc.cjs b/.eslintrc.cjs
index 8cdfdc2f383bc..b91860f0c54dc 100644
--- a/.eslintrc.cjs
+++ b/.eslintrc.cjs
@@ -7,8 +7,10 @@ module.exports = {
     '@typescript-eslint/explicit-module-boundary-types': 'off',
     '@typescript-eslint/no-use-before-define': 'off',
     '@typescript-eslint/no-var-requires': 'off',
+    'no-shadow': 'error',
     'prettier/prettier': 'error',
     'prefer-const': 'off',
     'prefer-rest-params': 'off',
+    'require-jsdoc': 'warn',
   },
 };
diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml
index 0808ce599db86..369d075631a51 100644
--- a/.github/workflows/nodejs.yml
+++ b/.github/workflows/nodejs.yml
@@ -27,3 +27,10 @@ jobs:
           npm test
         env:
           CI: true
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v1
+      - uses: actions/setup-node@v1
+      - run: npm ci
+      - run: npm run lint
diff --git a/snowpack-plugin.cjs b/snowpack-plugin.cjs
index dcb714ea61117..4a4db8efdfd52 100644
--- a/snowpack-plugin.cjs
+++ b/snowpack-plugin.cjs
@@ -9,7 +9,7 @@ module.exports = function (snowpackConfig, { resolve, extensions, astroConfig }
     knownEntrypoints: ['deepmerge'],
     resolve: {
       input: ['.astro', '.md'],
-      output: ['.js'],
+      output: ['.js', '.css'],
     },
     async load({ filePath }) {
       const { compileComponent } = await transformPromise;
@@ -21,7 +21,11 @@ module.exports = function (snowpackConfig, { resolve, extensions, astroConfig }
         extensions,
       };
       const result = await compileComponent(contents, { compileOptions, filename: filePath, projectRoot });
-      return result.contents;
+      const output = {
+        '.js': result.contents,
+      };
+      if (result.css) output['.css'] = result.css;
+      return output;
     },
   };
 };
diff --git a/src/@types/astro.ts b/src/@types/astro.ts
index ee6a79eabac47..5790c4b788275 100644
--- a/src/@types/astro.ts
+++ b/src/@types/astro.ts
@@ -25,9 +25,11 @@ export interface TransformResult {
   script: string;
   imports: string[];
   items: JsxItem[];
+  css?: string;
 }
 
 export interface CompileResult {
   result: TransformResult;
   contents: string;
+  css?: string;
 }
diff --git a/src/compiler/codegen.ts b/src/compiler/codegen.ts
index 70620111d82fe..63fc44dfbcfd0 100644
--- a/src/compiler/codegen.ts
+++ b/src/compiler/codegen.ts
@@ -21,7 +21,7 @@ interface Attribute {
   end: number;
   type: 'Attribute';
   name: string;
-  value: any;
+  value: TemplateNode[] | boolean;
 }
 
 interface CodeGenOptions {
@@ -41,7 +41,8 @@ function getAttributes(attrs: Attribute[]): Record<string, string> {
       result[attr.name] = JSON.stringify(attr.value);
       continue;
     }
-    if (attr.value === false) {
+    if (attr.value === false || attr.value === undefined) {
+      // note: attr.value shouldn’t be `undefined`, but a bad transform would cause a compile error here, so prevent that
       continue;
     }
     if (attr.value.length > 1) {
@@ -59,7 +60,7 @@ function getAttributes(attrs: Attribute[]): Record<string, string> {
         ')';
       continue;
     }
-    const val: TemplateNode = attr.value[0];
+    const val = attr.value[0];
     if (!val) {
       result[attr.name] = '(' + val + ')';
       continue;
@@ -72,7 +73,7 @@ function getAttributes(attrs: Attribute[]): Record<string, string> {
         result[attr.name] = JSON.stringify(getTextFromAttribute(val));
         continue;
       default:
-        throw new Error('UNKNOWN V');
+        throw new Error(`UNKNOWN: ${val.type}`);
     }
   }
   return result;
@@ -253,7 +254,7 @@ async function acquireDynamicComponentImports(plugins: Set<ValidExtensionPlugins
   return importMap;
 }
 
-export async function codegen(ast: Ast, { compileOptions, filename }: CodeGenOptions): Promise<TransformResult> {
+export async function codegen(ast: Ast, { compileOptions, filename, fileID }: CodeGenOptions): Promise<TransformResult> {
   const { extensions = defaultExtensions, astroConfig } = compileOptions;
   await eslexer.init;
 
@@ -334,6 +335,21 @@ export async function codegen(ast: Ast, { compileOptions, filename }: CodeGenOpt
   let collectionItem: JsxItem | undefined;
   let currentItemName: string | undefined;
   let currentDepth = 0;
+  let css: string[] = [];
+
+  walk(ast.css, {
+    enter(node: TemplateNode) {
+      if (node.type === 'Style') {
+        css.push(node.content.styles); // if multiple <style> tags, combine together
+        this.skip();
+      }
+    },
+    leave(node: TemplateNode) {
+      if (node.type === 'Style') {
+        this.remove(); // this will be optimized in a global CSS file; remove so it‘s not accidentally inlined
+      }
+    },
+  });
 
   walk(ast.html, {
     enter(node: TemplateNode) {
@@ -419,9 +435,9 @@ export async function codegen(ast: Ast, { compileOptions, filename }: CodeGenOpt
           return;
         }
         case 'Style': {
-          const attributes = getAttributes(node.attributes);
-          items.push({ name: 'style', jsx: `h("style", ${attributes ? generateAttributes(attributes) : 'null'}, ${JSON.stringify(node.content.styles)})` });
-          break;
+          css.push(node.content.styles); // if multiple <style> tags, combine together
+          this.skip();
+          return;
         }
         case 'Text': {
           const text = getTextFromAttribute(node);
@@ -469,6 +485,7 @@ export async function codegen(ast: Ast, { compileOptions, filename }: CodeGenOpt
           }
           return;
         case 'Style': {
+          this.remove(); // this will be optimized in a global CSS file; remove so it‘s not accidentally inlined
           return;
         }
         default:
@@ -481,5 +498,6 @@ export async function codegen(ast: Ast, { compileOptions, filename }: CodeGenOpt
     script: script,
     imports: Array.from(importExportStatements),
     items,
+    css: css.length ? css.join('\n\n') : undefined,
   };
 }
diff --git a/src/compiler/index.ts b/src/compiler/index.ts
index 8104ef4b4da38..a256bd9384bc4 100644
--- a/src/compiler/index.ts
+++ b/src/compiler/index.ts
@@ -10,7 +10,6 @@ import { CompileResult, TransformResult } from '../@types/astro';
 import { parse } from '../parser/index.js';
 import { createMarkdownHeadersCollector } from '../micromark-collect-headers.js';
 import { encodeMarkdown } from '../micromark-encode.js';
-import { defaultLogOptions } from '../logger.js';
 import { optimize } from './optimize/index.js';
 import { codegen } from './codegen.js';
 
@@ -75,14 +74,14 @@ async function convertMdToJsx(
 
   const raw = `---
   ${imports}
-  ${frontmatterData.layout ? `export const __layout = ${JSON.stringify(frontmatterData.layout)};` : ''}
+  ${frontmatterData.layout ? `import {__renderPage as __layout} from ${JSON.stringify('../../' + frontmatterData.layout)};` : 'const __layout = undefined;'}
   export const __content = ${stringifiedSetupContext};
 ---
 <section>${mdHtml}</section>`;
 
   const convertOptions = { compileOptions, filename, fileID };
 
-  return convertAstroToJsx(raw, convertOptions);
+  return await convertAstroToJsx(raw, convertOptions);
 }
 
 type SupportedExtensions = '.astro' | '.md';
@@ -94,9 +93,9 @@ async function transformFromSource(
   const fileID = path.relative(projectRoot, filename);
   switch (path.extname(filename) as SupportedExtensions) {
     case '.astro':
-      return convertAstroToJsx(contents, { compileOptions, filename, fileID });
+      return await convertAstroToJsx(contents, { compileOptions, filename, fileID });
     case '.md':
-      return convertMdToJsx(contents, { compileOptions, filename, fileID });
+      return await convertMdToJsx(contents, { compileOptions, filename, fileID });
     default:
       throw new Error('Not Supported!');
   }
@@ -108,8 +107,6 @@ export async function compileComponent(
 ): Promise<CompileResult> {
   const sourceJsx = await transformFromSource(source, { compileOptions, filename, projectRoot });
   const isPage = path.extname(filename) === '.md' || sourceJsx.items.some((item) => item.name === 'html');
-  // sort <style> tags first
-  sourceJsx.items.sort((a, b) => (a.name === 'style' && b.name !== 'style' ? -1 : 0));
 
   // return template
   let modJsx = `
@@ -144,8 +141,7 @@ export async function __renderPage({request, children, props}) {
 
   // find layout, if one was given.
   if (currentChild.layout) {
-    const layoutComponent = (await import('/_astro/layouts/' + currentChild.layout.replace(/.*layouts\\//, "").replace(/\.astro$/, '.js')));
-    return layoutComponent.__renderPage({
+    return currentChild.layout({
       request,
       props: {content: currentChild.content},
       children: [childBodyResult],
@@ -162,5 +158,6 @@ export async function __renderPage() { throw new Error("No <html> page element f
   return {
     result: sourceJsx,
     contents: modJsx,
+    css: sourceJsx.css,
   };
 }
diff --git a/src/compiler/optimize/styles.ts b/src/compiler/optimize/styles.ts
index fa32445bae399..25babedec692a 100644
--- a/src/compiler/optimize/styles.ts
+++ b/src/compiler/optimize/styles.ts
@@ -26,7 +26,7 @@ const getStyleType: Map<string, StyleType> = new Map([
 ]);
 
 const SASS_OPTIONS: Partial<sass.Options> = {
-  outputStyle: 'compressed',
+  outputStyle: process.env.NODE_ENV === 'production' ? 'compressed' : undefined,
 };
 /** HTML tags that should never get scoped classes */
 const NEVER_SCOPED_TAGS = new Set<string>(['html', 'head', 'body', 'script', 'style', 'link', 'meta']);
@@ -95,11 +95,10 @@ async function transformStyle(code: string, { type, filename, scopedClass }: { t
   return { css, type: styleType };
 }
 
+/** Style optimizer */
 export default function ({ filename, fileID }: { filename: string; fileID: string }): Optimizer {
   const styleNodes: TemplateNode[] = []; // <style> tags to be updated
   const styleTransformPromises: Promise<StyleTransformResult>[] = []; // async style transform results to be finished in finalize();
-  let rootNode: TemplateNode; // root node which needs <style> tags
-
   const scopedClass = `astro-${hashFromFilename(fileID)}`; // this *should* generate same hash from fileID every time
 
   return {
@@ -124,15 +123,7 @@ export default function ({ filename, fileID }: { filename: string; fileID: strin
               return;
             }
 
-            // 2. find the root node to inject the <style> tag in later
-            // TODO: remove this when we are injecting <link> tags into <head>
-            if (node.name === 'head') {
-              rootNode = node; // If this is <head>, this is what we want. Always take this if found. However, this may not always exist (it won’t for Component subtrees).
-            } else if (!rootNode) {
-              rootNode = node; // If no <head> (yet), then take the first element we come to and assume it‘s the “root” (but if we find a <head> later, then override this per the above)
-            }
-
-            // 3. add scoped HTML classes
+            // 2. add scoped HTML classes
             if (NEVER_SCOPED_TAGS.has(node.name)) return; // only continue if this is NOT a <script> tag, etc.
             // Note: currently we _do_ scope web components/custom elements. This seems correct?
 
@@ -175,10 +166,6 @@ export default function ({ filename, fileID }: { filename: string; fileID: strin
                 scopedClass,
               })
             );
-
-            // TODO: we should delete the old untransformed <style> node after we’re done.
-            // However, the svelte parser left it in ast.css, not ast.html. At the final step, this just gets ignored, so it will be deleted, in a sense.
-            // If we ever end up scanning ast.css for something else, then we’ll need to actually delete the node (or transform it to the processed version)
           },
         },
       },
@@ -186,14 +173,9 @@ export default function ({ filename, fileID }: { filename: string; fileID: strin
     async finalize() {
       const styleTransforms = await Promise.all(styleTransformPromises);
 
-      if (!rootNode) {
-        throw new Error(`No root node found`); // TODO: remove this eventually; we should always find it, but for now alert if there’s a bug in our code
-      }
-
-      // 1. transform <style> tags
       styleTransforms.forEach((result, n) => {
         if (styleNodes[n].attributes) {
-          // 1b. Inject final CSS
+          // 1. Replace with final CSS
           const isHeadStyle = !styleNodes[n].content;
           if (isHeadStyle) {
             // Note: <style> tags in <head> have different attributes/rules, because of the parser. Unknown why
@@ -202,22 +184,22 @@ export default function ({ filename, fileID }: { filename: string; fileID: strin
             styleNodes[n].content.styles = result.css;
           }
 
-          // 3b. Update <style> attributes
+          // 2. Update <style> attributes
           const styleTypeIndex = styleNodes[n].attributes.findIndex(({ name }: any) => name === 'type');
+          // add type="text/css"
           if (styleTypeIndex !== -1) {
             styleNodes[n].attributes[styleTypeIndex].value[0].raw = 'text/css';
             styleNodes[n].attributes[styleTypeIndex].value[0].data = 'text/css';
           } else {
             styleNodes[n].attributes.push({ name: 'type', type: 'Attribute', value: [{ type: 'Text', raw: 'text/css', data: 'text/css' }] });
           }
+          // remove lang="*"
           const styleLangIndex = styleNodes[n].attributes.findIndex(({ name }: any) => name === 'lang');
           if (styleLangIndex !== -1) styleNodes[n].attributes.splice(styleLangIndex, 1);
+          // add data-astro for later
+          styleNodes[n].attributes.push({ name: 'data-astro', type: 'Attribute', value: true });
         }
       });
-
-      // 2. inject finished <style> tags into root node
-      // TODO: pull out into <link> tags for deduping
-      rootNode.children = [...styleNodes, ...(rootNode.children || [])];
     },
   };
 }
diff --git a/src/runtime.ts b/src/runtime.ts
index 42024eff46e66..b527a45bb0112 100644
--- a/src/runtime.ts
+++ b/src/runtime.ts
@@ -1,8 +1,8 @@
-import type { SnowpackDevServer, ServerRuntime as SnowpackServerRuntime, LoadResult as SnowpackLoadResult, SnowpackConfig } from 'snowpack';
+import type { SnowpackDevServer, ServerRuntime as SnowpackServerRuntime, SnowpackConfig } from 'snowpack';
 import type { AstroConfig } from './@types/astro';
 import type { LogOptions } from './logger';
 import type { CompileError } from './parser/utils/error.js';
-import { info } from './logger.js';
+import { debug, info } from './logger.js';
 
 import { existsSync } from 'fs';
 import { loadConfiguration, logger as snowpackLogger, startServer as startSnowpackServer } from 'snowpack';
@@ -39,7 +39,6 @@ async function load(config: RuntimeConfig, rawPathname: string | undefined): Pro
 
   const selectedPageLoc = new URL(`./pages/${selectedPage}.astro`, astroRoot);
   const selectedPageMdLoc = new URL(`./pages/${selectedPage}.md`, astroRoot);
-  const selectedPageUrl = `/_astro/pages/${selectedPage}.js`;
 
   // Non-Astro pages (file resources)
   if (!existsSync(selectedPageLoc) && !existsSync(selectedPageMdLoc)) {
@@ -62,49 +61,66 @@ async function load(config: RuntimeConfig, rawPathname: string | undefined): Pro
     }
   }
 
-  try {
-    const mod = await snowpackRuntime.importModule(selectedPageUrl);
-    let html = (await mod.exports.__renderPage({
-      request: {
-        host: fullurl.hostname,
-        path: fullurl.pathname,
-        href: fullurl.toString(),
-      },
-      children: [],
-      props: {},
-    })) as string;
-
-    // inject styles
-    // TODO: handle this in compiler
-    const styleTags = Array.isArray(mod.css) && mod.css.length ? mod.css.reduce((markup, url) => `${markup}\n<link rel="stylesheet" type="text/css" href="${url}" />`, '') : ``;
-    if (html.indexOf('</head>') !== -1) {
-      html = html.replace('</head>', `${styleTags}</head>`);
-    } else {
-      html = styleTags + html;
-    }
+  for (const url of [
+    `/_astro/pages/${selectedPage}.astro.js`,
+    `/_astro/pages/${selectedPage}.md.js`,
+    `/_astro/pages/${selectedPage}.astro.css`,
+    `/_astro/pages/${selectedPage}.md.css`,
+  ]) {
+    try {
+      const mod = await snowpackRuntime.importModule(url);
+      debug(logging, 'resolve', `${reqPath} -> ${url}`);
+      let html = (await mod.exports.__renderPage({
+        request: {
+          host: fullurl.hostname,
+          path: fullurl.pathname,
+          href: fullurl.toString(),
+        },
+        children: [],
+        props: {},
+      })) as string;
+
+      // inject styles
+      // TODO: handle this in compiler
+      const styleTags = Array.isArray(mod.css) && mod.css.length ? mod.css.reduce((markup, url) => `${markup}\n<link rel="stylesheet" type="text/css" href="${url}" />`, '') : ``;
+      if (html.indexOf('</head>') !== -1) {
+        html = html.replace('</head>', `${styleTags}</head>`);
+      } else {
+        html = styleTags + html;
+      }
 
-    return {
-      statusCode: 200,
-      contents: html,
-    };
-  } catch (err) {
-    switch (err.code) {
-      case 'parse-error': {
-        return {
-          statusCode: 500,
-          type: 'parse-error',
-          error: err,
-        };
+      return {
+        statusCode: 200,
+        contents: html,
+      };
+    } catch (err) {
+      // if this is a 404, try the next URL (will be caught at the end)
+      const notFoundError = err.toString().startsWith('Error: Not Found') || err.toString().includes(`Requested content ".css" but built .js`);
+      if (notFoundError) {
+        continue;
       }
-      default: {
+
+      if (err.code === 'parse-error') {
         return {
           statusCode: 500,
-          type: 'unknown',
+          type: 'parse-error',
           error: err,
         };
       }
+      return {
+        statusCode: 500,
+        type: 'unknown',
+        error: err,
+      };
     }
   }
+
+  // couldn‘t find match; 404
+  return {
+    statusCode: 404,
+    type: 'unknown',
+    error: new Error(`Could not locate ${selectedPage}`),
+  };
 }
 
 export interface AstroRuntime {
diff --git a/test/astro-styles-ssr.test.js b/test/astro-styles-ssr.test.js
index 9ee67f69bc00f..2342fcda48349 100644
--- a/test/astro-styles-ssr.test.js
+++ b/test/astro-styles-ssr.test.js
@@ -8,6 +8,16 @@ const StylesSSR = suite('Styles SSR');
 
 let runtime;
 
+/** Basic CSS minification; removes some flakiness in testing CSS */
+function cssMinify(css) {
+  return css
+    .trim() // remove whitespace
+    .replace(/\n\s*/g, '') // collapse lines
+    .replace(/\s*\{/g, '{') // collapse selectors
+    .replace(/:\s*/g, ':') // collapse attributes
+    .replace(/;}/g, '}'); // collapse block
+}
+
 StylesSSR.before(async () => {
   const astroConfig = await loadConfig(new URL('./fixtures/astro-styles-ssr', import.meta.url).pathname);
 
@@ -54,12 +64,15 @@ StylesSSR('CSS Module support in .astro', async () => {
   let scopedClass;
 
   // test 1: <style> tag in <head> is transformed
-  const css = $('style')
-    .html()
-    .replace(/\.astro-[A-Za-z0-9-]+/, (match) => {
-      scopedClass = match;
-      return match;
-    }); // remove class hash (should be deterministic / the same every time, but even still don‘t cause this test to flake)
+  const css = cssMinify(
+    $('style')
+      .html()
+      .replace(/\.astro-[A-Za-z0-9-]+/, (match) => {
+        scopedClass = match; // get class hash from result
+        return match;
+      })
+  );
+
   assert.equal(css, `.wrapper${scopedClass}{margin-left:auto;margin-right:auto;max-width:1200px}`);
 
   // test 2: element received .astro-XXXXXX class (this selector will succeed if transformed correctly)