Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix amp css #286

Merged
merged 3 commits into from
Dec 17, 2020
Merged

Fix amp css #286

merged 3 commits into from
Dec 17, 2020

Conversation

Rich-Harris
Copy link
Member

#279 fixed some stuff but broke AMP CSS, because AMP introduces a tricky requirement.

Ordinarily, we just add <link rel="stylesheet"> elements, which is easy to do in both dev (because we extract dependencies on the fly as pages are rendered) and prod (because the bundling step produces a manifest). But AMP insists that you inline all the page's CSS in a <style amp-custom> element, which means we need to get the contents of those files, which is easy enough in dev — just call snowpack.loadUrl — but a bit harder in prod because the CSS files might be hosted somewhere else.

The solution here is to inline the CSS contents rather than the URLs. (In fact in the AMP case we ideally wouldn't be generating client-side assets at all, but that would involve some larger refactoring than I can get into just now.) I don't love it — it could result in a chunky slow-to-start function, if you weren't careful — but I don't have a great alternative right now, and I need this to work, so...

Anyway. Inlining CSS is a smart thing to do below a certain threshold, so it would be good to come up with a practical solution for this that wasn't limited to AMP documents (and ideally one that didn't involve fs given #223).

@Rich-Harris
Copy link
Member Author

Enabled "editor.formatOnSave" in the VSCode workspace settings, because we're losing our precious github actions minutes to the linter

@Rich-Harris Rich-Harris merged commit 6d1bb11 into master Dec 17, 2020
@Rich-Harris Rich-Harris deleted the fix-amp-css branch December 17, 2020 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants