Inline CSS with Critters Plugin #642

lukeed · 2018-09-08T22:59:48Z

What kind of change does this PR introduce?

Feature

Did you add tests for your changes?

No

Summary

Uses Critters to inline critical CSS on the client bundle(s) for production and development.

The default settings are good 👌 but can be changed by end-user thru preact.config.js if/when needed.

Closes #637

Does this PR introduce a breaking change?

No

prateekbh · 2018-09-09T05:06:19Z

Quick thing, I'd like the bundle.css to defer loaded since this is inlined anyways. WDYT?

lukeed · 2018-09-09T07:14:36Z

Unless I've missed something, both defer and async are <script>-specific attributes... they won't do anything (for now) on other tags.

The default Critters setup uses rel=preload as=style which is as close to defer as you can get me thinks, before bringing in any JS

prateekbh · 2018-09-09T12:55:44Z

Hmmm, how about we inline what critters give us and put bundle.css as
<line rel='defer-stylesheet' href='...'> and use js to load this later on?

That should make our first-paint super fast

lukeed · 2018-09-09T19:31:51Z

I think our first-paint is already super fast with default setting. Don't think we should be doing anything custom/beyond what Critters offers, as that's the point/goal of Critters, no?

Either way, sounds like you may want the { preload: 'swap' } option.

reznord

LGTM 👍

prateekbh · 2018-09-10T13:53:41Z

Oh I just read:

default: Move stylesheet links to the end of the document and insert preload meta tags in their place.

This seems fine, if it is indeed moving it to the end. Please just check that we don't have double preload css tags with --preload

prateekbh · 2018-09-11T01:16:38Z

Also, tried this right now and running into: GoogleChromeLabs/critters#5

packages/cli/lib/lib/webpack/webpack-client-config.js

prateekbh

Yes it does put duplicate preload tags with --preload for bundle.css. So that should be avoided if this plugin is in by default.

Also I still get the Defer unused CSS lighthouse error, so may be swap is a better option

prateekbh · 2018-09-11T16:30:07Z

btw there should be an opt-out/opt-in flag for critters because based on your css critters is running into edge cases.

cc: @developit, posted one such on the bug thread in critters

lukeed · 2018-09-12T00:30:06Z

Fixed the plugin ordering 👍

There are double link rel=preload tags, but that shouldn't matter? It still only fires off a single request to bundle.*.css & I'm not getting any unused warnings.

lukeed · 2018-09-12T00:47:56Z

I think I see what you're talking about.

The default mode append a <link rel=stylesheet href="bundle.*.css"/> to the end of the document – is that what you were referring to?

The tag is removed with js and js-lazy, but has no effect on the Lighthouse score w/ our Preact templates since the CSS is so little.

prateekbh · 2018-09-12T00:56:26Z

The tag is removed with js and js-lazy, but has no effect on the Lighthouse score w/ our Preact templates since the CSS is so little.

This has affect when the app grows e.g. material.preact.com

preaload: swap works pretty perfect with just a few characters even for bigger sites.

There are double link rel=preload tags, but that shouldn't matter?

Chrome throws a warning for this in console IIRC

lukeed · 2018-09-12T01:04:27Z

Gotcha. It warns if unused, but not on double preload tag.

Feel free to change it to swap, I don't have any objections! I'll likely be away from desk for a few hours

prateekbh · 2018-09-12T02:11:58Z

How about along with a opt-out flag?

lukeed · 2018-09-12T02:17:24Z

Personally I don't like opt-out flags, so I'd be hesitant. I also not sure why you'd need one?

Arguably, if we're making something a default behavior I feel that we should stand behind it 100©

Unlike the esm feature, there's no compatibility or support issues.

prateekbh · 2018-09-12T02:46:28Z

Because I feel that it's not very stable, I made like 4-5 changes to the source code in my node_modules before I could get it to work

prateekbh · 2018-09-13T05:41:36Z

@lukeed @developit should we be concerned about the fact that even after using preload(swap), latest lighthouse still complains about unused for the link tag and even the inlined css is being reported as extra css supplied

e.g. https://www.webpagetest.org/lighthouse.php?test=180913_MG_dc5a8260f5c19115230143e77c18527d&run=1

prateekbh · 2018-09-13T12:13:04Z

Also, another use case of opt-out flag/question: if I use a case in July aren't they better at giving me my critical css rather than critters?

lukeed · 2018-09-14T06:13:18Z

If it's not stable, then:

a) we shouldn't include it
b) we should fix it so that it's stable; or
c) enable the feature with a flag, not disable with flag

prateekbh · 2018-09-14T06:25:36Z

a) we shouldn't include it

We should as it'll give us great perf boost for whomsoever it works

b) we should fix it so that it's stable; or

I'll wrap my finding in a PR to critters, but have no ETA on this.

c) enable the feature with a flag, not disable with flag

Agreed, do you wanna do this?

lukeed · 2018-09-14T06:29:29Z

Oops, sorry, I meant that those were three possible choices; not to do all three 😅

I'd be totally stoked with inline-css as a default feature, but we need to be confident in it. If we're not, then it either can't be a default behavior or shouldn't be there at all

prateekbh · 2018-09-14T09:08:19Z

+1, does an opt in flag sound like a decent approach now?

ForsakenHarmony · 2018-09-14T11:31:48Z

We could do it like the esm stuff

Opt in now and if we solve all the problems we can make it the default

prateekbh · 2018-09-15T22:44:43Z

added a flag, and conditional preload of css if critters is used

lukeed · 2018-09-15T22:48:00Z

packages/cli/lib/index.js

@@ -31,7 +31,8 @@ prog
 	.option('--analyze', 'Launch interactive Analyzer to inspect production bundle(s)')
 	.option('--prerenderUrls', 'Path to pre-rendered routes config', 'prerender-urls.json')
 	.option('-c, --config', 'Path to custom CLI config', 'preact.config.js')
-	.option('--esm', '[EXPERIMENTAL] Builds ES-2015 bundles for your code.', false)
+	.option('--esm', 'Builds ES-2015 bundles for your code.', false)
+	.option('--critical-css', 'Adds critical cssto the prerendered markup.', true)


I would make this --inline or --inline-css, but maybe that's just me /shrug

Nah that sounds better

lukeed · 2018-09-15T22:49:26Z

packages/cli/lib/resources/template.html

@@ -10,8 +10,9 @@
 		<% if (htmlWebpackPlugin.options.manifest.theme_color) { %>
 			<meta name="theme-color" content="<%= htmlWebpackPlugin.options.manifest.theme_color %>">
 		<% } %>
+		<% const filesRegexp = options.criticalCss ? /\.(js)$/ : /\.(css|js)$/;%>


I don't understand – we'd still want to preload some CSS, especially the routes' chunks.

lukeed · 2018-09-15T23:34:01Z

@prateekbh FYI - failing tests are now actually relevant to your changes

… into feat/critters

…eat/critters

prateekbh · 2018-09-16T00:23:45Z

It passes now but it shouldn't, I haven't fixed the preload tests don't know why they are passing

prateekbh · 2018-09-16T19:40:37Z

fixed everything, good to merge. plz review

Dismissing my own review as I added commits

lukeed

I can't approve since I initiated the PR 😆 Looks good!

prateekbh · 2018-09-16T20:26:06Z

now @developit also has a commit 😆

developit · 2018-09-16T21:17:16Z

Critters issue should be fixed in 1.2.0.

prateekbh · 2018-09-16T22:03:11Z

@ForsakenHarmony can you review this one :)

reznord · 2018-09-17T07:27:47Z

packages/cli/lib/resources/template.html

@@ -10,8 +10,9 @@
 		<% if (htmlWebpackPlugin.options.manifest.theme_color) { %>
 			<meta name="theme-color" content="<%= htmlWebpackPlugin.options.manifest.theme_color %>">
 		<% } %>
+		<% const filesRegexp = htmlWebpackPlugin.options.inlineCss ? /\.(chunk\.\w{5}\.css|js)$/ : /\.(css|js)$/;%>


template.html is already complicated and this adds more to it 🙈.

@developit any plans on making it similar to what CRA does?

Well I can take a look at this in next iteration, but not for this PR

ForsakenHarmony

LGTM

use critters-webpack-plugin in client config

694e029

reznord approved these changes Sep 10, 2018

View reviewed changes

prateekbh reviewed Sep 11, 2018

View reviewed changes

packages/cli/lib/lib/webpack/webpack-client-config.js Outdated Show resolved Hide resolved

prateekbh previously requested changes Sep 11, 2018

View reviewed changes

fix: move Critters after HTML

b655ad1

prateekbh added 3 commits September 15, 2018 15:35

adding a flag for critters and preload swap

fd83230

conditional preload for css

f02d7a2

indentation

8a999a9

prateekbh force-pushed the feat/critters branch from 1e80b3b to 8a999a9 Compare September 15, 2018 22:44

lukeed commented Sep 15, 2018

View reviewed changes

fix linter

e96a9b7

prateekbh added 3 commits September 15, 2018 17:04

fixing regexp and flag name

674a01c

Merge branch 'feat/critters' of https://github.com/developit/preact-cli…

fad01cc

… into feat/critters

Merge branch 'next' of https://github.com/developit/preact-cli into f…

ff35bc5

…eat/critters

fixing tests

63f2e8d

lint fix try1

1ab0da9

lukeed commented Sep 16, 2018

View reviewed changes

spellung

d50d68b

prateekbh force-pushed the feat/critters branch from 6c8c812 to d50d68b Compare September 16, 2018 22:01

Update package.json

3b62a43

reznord reviewed Sep 17, 2018

View reviewed changes

ForsakenHarmony approved these changes Sep 17, 2018

View reviewed changes

hassanbazzi approved these changes Sep 17, 2018

View reviewed changes

hassanbazzi merged commit 53153f1 into next Sep 17, 2018

lukeed deleted the feat/critters branch September 17, 2018 22:37

Inline CSS with Critters Plugin #642

Inline CSS with Critters Plugin #642

Conversation

lukeed commented Sep 8, 2018

prateekbh commented Sep 9, 2018

lukeed commented Sep 9, 2018

prateekbh commented Sep 9, 2018 • edited Loading

lukeed commented Sep 9, 2018

reznord left a comment • edited Loading

Choose a reason for hiding this comment

prateekbh commented Sep 10, 2018

prateekbh commented Sep 11, 2018

prateekbh left a comment • edited Loading

Choose a reason for hiding this comment

prateekbh commented Sep 11, 2018 • edited Loading

lukeed commented Sep 12, 2018

lukeed commented Sep 12, 2018

prateekbh commented Sep 12, 2018

lukeed commented Sep 12, 2018

prateekbh commented Sep 12, 2018

lukeed commented Sep 12, 2018 • edited Loading

prateekbh commented Sep 12, 2018

prateekbh commented Sep 13, 2018 • edited Loading

prateekbh commented Sep 13, 2018

lukeed commented Sep 14, 2018

prateekbh commented Sep 14, 2018

lukeed commented Sep 14, 2018

prateekbh commented Sep 14, 2018

ForsakenHarmony commented Sep 14, 2018 • edited Loading

prateekbh commented Sep 15, 2018

lukeed Sep 15, 2018

Choose a reason for hiding this comment

prateekbh Sep 15, 2018

Choose a reason for hiding this comment

lukeed Sep 15, 2018

Choose a reason for hiding this comment

lukeed commented Sep 15, 2018

prateekbh commented Sep 16, 2018

prateekbh commented Sep 16, 2018

lukeed left a comment

Choose a reason for hiding this comment

prateekbh commented Sep 16, 2018

developit commented Sep 16, 2018

prateekbh commented Sep 16, 2018

reznord Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

prateekbh Sep 17, 2018

Choose a reason for hiding this comment

ForsakenHarmony left a comment

Choose a reason for hiding this comment

prateekbh commented Sep 9, 2018 •

edited

Loading

reznord left a comment •

edited

Loading

prateekbh left a comment •

edited

Loading

prateekbh commented Sep 11, 2018 •

edited

Loading

lukeed commented Sep 12, 2018 •

edited

Loading

prateekbh commented Sep 13, 2018 •

edited

Loading

ForsakenHarmony commented Sep 14, 2018 •

edited

Loading

reznord Sep 17, 2018 •

edited

Loading