Extract Astro styles to external stylesheets #43

drwpow · 2021-03-30T21:38:39Z

Changes

Pulls out stylesheets for .astro files
Pulls out stylesheets for .md files

Testing

Tests are passing
Tests updated where necessary

Docs

Docs / READMEs updated
Code comments added where helpful

src/compiler/index.ts

drwpow · 2021-03-30T23:15:43Z

src/compiler/optimize/styles.ts

 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 });


So an interesting point: in astro dev: for all <style> tags found in /astro/pages/*.astro, I could not find a way to pull that out and load an external .css file. In fact, it may not even be that bad—that something you write inline in a page, gets inlined.

Either way, what I’ve done is mark it with a <style data-astro> attribute here. When we build, that will give us a marker in case we want to optimize these specific style tags after-the-fact.

Yea, I kind of like that! A bit surprised that wasn't the default but I'm probably missing something obvious about how components render vs. how pages render.

Do we use the data-astro attribute right now? Lets remove this for now if its not needed. Feel free to add as a comment instead. Something like: // TODO: add data-astro attribute to track in the optimize step.

Yeah I couldn‘t pin it down exactly, but some way in which we compile the <head> element in Astro, outside of Snowpack, means we‘d have to extract those styles a different way. And possibly do another DOM walk, after the h() tags are rendered cc @matthewp. But I figured we could do that as part of build, and leave dev inlined as it is currently (again, all .astro components are still extracting their styles successfully)

drwpow · 2021-03-30T23:16:11Z

src/runtime.ts

+ ]) {
+ try {
+ const mod = await snowpackRuntime.importModule(url);
+ debug(logging, 'resolve', `${reqPath} -> ${url}`);


Now since we‘re trying several URLs in a row, I thought it‘d be good to show which source file we resolved from.

if we're loading something like /foo, we're only interested in foo.md.js and foo.astro.js right? The CSS file would never have a __renderPage() function to call below, and it would fail there instead I think.

Ah yes you‘re correct. Will remove

drwpow · 2021-03-30T23:17:11Z

.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',


I added the coveted no-shadow lint rule here to help us later

drwpow · 2021-03-30T23:17:22Z

.eslintrc.cjs

 'prettier/prettier': 'error',
 'prefer-const': 'off',
 'prefer-rest-params': 'off',
+ 'require-jsdoc': 'warn',


Also added this lint rule, but as a warning

drwpow · 2021-03-30T23:19:35Z

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'],


This is basically the core change here. Most of the other stuff is just unbreaking what this breaks

drwpow · 2021-03-30T23:20:04Z

src/compiler/index.ts

 export const __content = ${stringifiedSetupContext};
 ---
 <section>${mdHtml}</section>`;

 const convertOptions = { compileOptions, filename, fileID };

- return convertAstroToJsx(raw, convertOptions);
+ return await convertAstroToJsx(raw, convertOptions);


Added some awaits because why not

drwpow · 2021-03-30T23:20:24Z

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,


Minor change, but we don‘t need to slow down styles in dev like this

Oh an issue in Snowpack that I don't want to repeat in Astro: can we have an explicit "mode" instead of reading from NODE_ENV like this? It causes trouble when you want to add a feature like build --watch.

Feel free to move into a different PR if that feels out of scope here.

I almost added an env option that gets passed into the runtime but wound up not needing it so +1 on adding that.

One nice about Astro so far is that we don't really rely on globals at all, everything is passed through functions, even logging.

Will do! But yeah I’ll add a separate PR in case tests need to be updated, etc etc.

FredKSchott · 2021-03-30T23:44:42Z

.github/workflows/nodejs.yml

+ - uses: actions/checkout@v1
+ - uses: actions/setup-node@v1
+ - run: npm ci
+ - run: npm run lint


FredKSchott

this is my first time seeing a GH lint error inline like that!

That @ts-ignore warning seems aggressive, can we turn that off?

FredKSchott · 2021-03-30T23:46:48Z

src/compiler/codegen.ts

@@ -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


should we log this?

I’d lean towards “no“ as this is missing context and wouldn‘t be helpful (i.e. it couldn‘t show you bad syntax in your code). IMO this is a weird part of the code; if an error is thrown here, then context is already lost where the bad thing happened. I’d rather just use TypeScript types more stringently; throwing or logging here just says “you messed up somewhere else IDK”

Also to be clear: in the event that something else messed up, output should be affected, i.e. we should catch it in our tests.

src/compiler/index.ts

FredKSchott · 2021-03-30T23:51:27Z

src/compiler/index.ts

@@ -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({


if for some reason there was some CSS in this file, can we pass it through to currentChild.layout({css})?

Maybe? Personally I‘d rather tackle “support <style> in Markdown” as a separate thing if that‘s OK. I’m +1 for it but there may be more required there (hopefully not—hopefully it is as simple as adding it here). Adding a ticket to track this.

FredKSchott · 2021-03-30T23:52:55Z

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,


Oh an issue in Snowpack that I don't want to repeat in Astro: can we have an explicit "mode" instead of reading from NODE_ENV like this? It causes trouble when you want to add a feature like build --watch.

Feel free to move into a different PR if that feels out of scope here.

FredKSchott · 2021-03-30T23:54:50Z

src/compiler/optimize/styles.ts

 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 });


Yea, I kind of like that! A bit surprised that wasn't the default but I'm probably missing something obvious about how components render vs. how pages render.

Do we use the data-astro attribute right now? Lets remove this for now if its not needed. Feel free to add as a comment instead. Something like: // TODO: add data-astro attribute to track in the optimize step.

FredKSchott · 2021-03-30T23:57:12Z

src/runtime.ts

+ ]) {
+ try {
+ const mod = await snowpackRuntime.importModule(url);
+ debug(logging, 'resolve', `${reqPath} -> ${url}`);


if we're loading something like /foo, we're only interested in foo.md.js and foo.astro.js right? The CSS file would never have a __renderPage() function to call below, and it would fail there instead I think.

FredKSchott · 2021-03-31T00:03:49Z

src/runtime.ts

+ };
+ } 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`);


I'd rather not put too much logic into this one big global error handler if we can help it. can we move the "find the right source file" step out of this for loop? Something like:

function getFileForPath(reqPath: string) { const selectedPage = reqPath.substr(1) || 'index'; for (const url of [`/_astro/pages/${selectedPage}.astro.js`, `/_astro/pages/${selectedPage}.md.js`]) { try { return await snowpackRuntime.importModule(url); // or maybe return the matching file loc? feels like a waste to throw mod away if we'll load it again next // maybe call snowpack.load() instead just to see if it exists? catch (err) { if (err.toString().startsWith('Error: Not Found')) { continue; } throw err; } } return null; }

matthewp · 2021-03-31T14:00:23Z

src/compiler/codegen.ts

@@ -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, {


Nice, I think this is exactly the right place to do this, this seems pretty clean.

drwpow force-pushed the astro-css branch 2 times, most recently from 48a7533 to f2cf49d Compare March 30, 2021 22:06

drwpow changed the title ~~[wip] Extract Astro styles to external stylesheets~~ Extract Astro styles to external stylesheets Mar 30, 2021

drwpow force-pushed the astro-css branch 2 times, most recently from c6d8009 to b6af52e Compare March 30, 2021 23:02

drwpow requested a review from FredKSchott March 30, 2021 23:02

drwpow commented Mar 30, 2021

View reviewed changes

src/compiler/index.ts Outdated Show resolved Hide resolved

drwpow force-pushed the astro-css branch from b6af52e to 0fa772b Compare March 30, 2021 23:05

drwpow commented Mar 30, 2021

View reviewed changes

FredKSchott reviewed Mar 30, 2021

View reviewed changes

.github/workflows/nodejs.yml

- uses: actions/checkout@v1

- uses: actions/setup-node@v1

- run: npm ci

- run: npm run lint

Copy link

Member

FredKSchott Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

FredKSchott reviewed Mar 30, 2021

View reviewed changes

FredKSchott approved these changes Mar 31, 2021

View reviewed changes

matthewp reviewed Mar 31, 2021

View reviewed changes

drwpow force-pushed the astro-css branch 4 times, most recently from 41da487 to f00f203 Compare March 31, 2021 17:42

Extract Astro styles to external stylesheets

3b8277d

drwpow force-pushed the astro-css branch from f00f203 to 3b8277d Compare March 31, 2021 17:44

Require relative URLs in Markdown layouts

ee411ec

drwpow merged commit 3fa6396 into main Mar 31, 2021

drwpow deleted the astro-css branch March 31, 2021 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Astro styles to external stylesheets #43

Extract Astro styles to external stylesheets #43

drwpow commented Mar 30, 2021 •

edited

Loading

drwpow Mar 30, 2021

FredKSchott Mar 30, 2021

drwpow Mar 31, 2021

drwpow Mar 30, 2021

FredKSchott Mar 30, 2021

drwpow Mar 31, 2021

drwpow Mar 30, 2021

drwpow Mar 30, 2021

drwpow Mar 30, 2021

matthewp Mar 31, 2021

drwpow Mar 30, 2021

drwpow Mar 30, 2021

FredKSchott Mar 30, 2021

matthewp Mar 31, 2021

drwpow Mar 31, 2021

FredKSchott Mar 30, 2021

FredKSchott left a comment •

edited

Loading

FredKSchott Mar 30, 2021

drwpow Mar 31, 2021

drwpow Mar 31, 2021 •

edited

Loading

FredKSchott Mar 30, 2021

drwpow Mar 31, 2021

FredKSchott Mar 30, 2021

FredKSchott Mar 30, 2021

FredKSchott Mar 30, 2021

FredKSchott Mar 31, 2021

matthewp Mar 31, 2021

Extract Astro styles to external stylesheets #43

Extract Astro styles to external stylesheets #43

Conversation

drwpow commented Mar 30, 2021 • edited Loading

Changes

Testing

Docs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow commented Mar 30, 2021 •

edited

Loading

FredKSchott left a comment •

edited

Loading

drwpow Mar 31, 2021 •

edited

Loading