Migrate to headless Chrome?

[addyosmani]

Now that headless Chrome is a thing and the DevTools support extracting unused CSS via the CSS/JS Code Coverage tool, it may be worth considering if critical would like to switch away from using PhantomJS over to this as part of a rewrite.

CSS Code Coverage is available via the remote debugging protocol and my understanding is that we could programmatically use this to extract out CSS in a more accurate manner while maintaining a similar workflow to what critical supports today.

Something that could be of value to consider is would this change be worth the effort? Would switching away from Phantom under the hood and over to the coverage model improve how close to ideal we're able to extract critical styles? or is the time we could spend doing this better spent addressing the existing edge-cases that come up in issues and smoothing those over instead?

[prateekbh]

idea of having css extracted with the help of coverage tool is really good as I guess that is what lighthouse uses. Also users rely on it as well. Although what happens to dynamic classnames?
ripple classes, hidden dialog classes etc.. With coverage tool I record the activity and run tasks the most necessary actions on page and then see the results, not sure how we deal with this in the most generic way

[addyosmani]

To be honest, I'm still somewhat skeptical of the value of code coverage tooling that doesn't include some form of UI hit-testing. That said, even today tools like critical need to work around this by allowing authors to specify their dynamic styles separately or come up with a whitelist of dynamic styles that aren't going to be removed. I could see us using headless + coverage while still having to expose these workarounds for the end user.

[bezoerb]

I think it's worth a try. We should give it a shot on a separate branch to see how far we get.

[kurtextrem]

Don't we have snippets that generate critical css? If I'm not mistaken Chrome headless is able to inject scripts into pages - what if we combine the coverage report with those snippets?

[pocketjoso]

Just FYI I'm now looking to migrate Penthouse to headless chrome, so if you wait another ~ 2 weeks you can get this migration without any changes to critical, aside from a Penthouse version bump. 👊

[addyosmani]

@pocketjoso Wow. That's...pretty incredible :) Thanks for keeping an eye on the repo and for letting us know! That'll save a whole lot of time! We really appreciate you working on that migration 🎉 🍰

[pocketjoso]

I have a headless chrome version of Penthouse with feature parity now, just fiddling with support for Linux etc - if you want to play with it it's available on the next tag (penthouse@next), currently as penthouse@rc-4. I will take some time before shipping this as latest.
(penthouse) pr is here: pocketjoso/penthouse#174

[bezoerb]

@pocketjoso: Thats awesome! Thanks for pushing things forward :)

[reznord]

@pocketjoso: That's super cool. Will try it on a different branch and test it.

[reznord]

I will be working on it in a separate branch in my fork.

[phaistonian]

So let me get it.

Is there a way we can use Critical with headless chrome now?

ps: I am using headless Chrome any chance I get - it feels NOT dirty, fast and overall awesome.

[pocketjoso]

@phaistonian maybe you can just manually replace the penthouse lib in your node_modules with the one from penthouse@next - if you can get critical to run with it it will work out of the box, as I made the API backwards compatible.

[pocketjoso]

@bezoerb @addyosmani and others:

I will release Penthouse 1.0 this week (using chrome headless via Puppeteer); I've just been writing up a release blog post, and waiting for the Puppeteer team to close a few important bugs.

1.0 will be backwards compatible (except for that Puppeteer requires Node>=6.4.0), so this means you can upgrade without changing a thing! (the reason for the major version bump is more to mark Penthouse as stable tech).

I recommend taking advantage of some changes though:

• Use cssString instead of css (filepath), to avoid unnecessary i/o.
• Parallel execution now works, so you might want to utilise this better. Now concurrent penthouse calls get run in different tabs in chromium, rather than as separate browser instances, as was previously the case. This does not only make execution much faster, but also avoid cases where PhantomJS would occasionally just crash, even without hitting resource limits. Note that in the default settings the shared browser instance will get closed as soon as there is no pending call.
• optional; perhaps not relevant for you - Penthouse 1.0 supports generating before/after screenshots (with/without critical css - in both cases the page rendered without JS), these can be used to increase confidence that the critical css is correct.
• get rid of const penthouseAsync = Bluebird.promisify(penthouse), as Penthouse has been returning promises since 0.11.0
• test using file:/// protocol for local files via Chrome headless - which hopefully works across platforms and hence can remove the need for critical to be [starting local servers for the generation])https://github.com/addyosmani/critical/blame/master/lib/core.js#L207)

You can check the readme from the chrome headless Penthouse PR; it is mostly up to date and I will fix the remaining this week.

As I've said before you can test this version on penthouse@next, but at this stage I've run it in production for more than a week and generated over 100,000 critical css files - it's definitely in better shape in every way than the previous version of penthouse.

[phaistonian]

@pocketjoso Worked like charm!

One little thing:

I get this

 UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 3): Error: Protocol error (Target.closeTarget): Target closed.

all the time. It doe not affect the result, but it's a bit annoying.

[reznord]

@phaistonian When I test the same in my local, two tests are failing for me. Here is the error log.

[pocketjoso]

So this is when running a lot of invocations in sequence, am I right? not in parallel? I'll look into it - thanks for testing both of you!

[reznord]

Yup.

[phaistonian]

Having used the lighthouse/chrome-launcher/chrome-launcher (not Puppeteer) to perform some related tasks, I think the issue has to do with the fact that there's a new task launched before the previous Target (Protocol) is closed.

Perhaps an "await" would do the trick :)

[pocketjoso]

@phaistonian Well the intention is to keep the browser instance running, to save some time and resources, but it currently closes as soon as the puppeteer task finishes - so before the next call happens when you run them in sequence. Sounds like there's some timing issue when the two calls happen close after each other (but not in parallel).

[addyosmani]

I will release Penthouse 1.0 this week (using chrome headless via Puppeteer); I've just been writing up a release blog post, and waiting for the Puppeteer team to close a few important bugs.

\o/ This is amazing work, @pocketjoso. I can't thank you enough for the tireless work in getting Penthouse updated to use headless Chrome. I sit across from the Puppeteer team and if you find yourself blocked on anything at all, please feel free to ping and I'll have a chat with them.

1.0 will be backwards compatible (except for that Puppeteer requires Node>=6.4.0), so this means you can upgrade without changing a thing! (the reason for the major version bump is more to mark Penthouse as stable tech).

I had been very curious to what extent backward compat would be maintained, but this sounds great.

@phaistonian When I test the same in my local, two tests are failing for me. Here is the error log.

Same here. This appears to occur during sequential invocations rather than running in parallel.

As I've said before you can test this version on penthouse@next, but at this stage I've run it in production for more than a week and generated over 100,000 critical css files - it's definitely in better shape in every way than the previous version of penthouse.

This is a good confidence booster. I believe @reznord is interested in working on @next support for critical and might put together a PR for us to switch over as a next step.

optional; perhaps not relevant for you - Penthouse 1.0 supports generating before/after screenshots (with/without critical css - in both cases the page rendered without JS), these can be used to increase confidence that the critical css is correct.

Whoa whoa whoa. That's actually very interesting. A part of my workflow when using critical has always been to do the comparison of the before and after. I'd been wondering if it made sense for a mode that would generate before/after screenshots and do an image diff or visual similarity diff (using SSIM) to inform the user whether the output was 100% as expected. Your work here sounds like it would provide at least the first part of this and I'd love to make sure our readme reflects how to use it with critical via Penthouse.

[pocketjoso]

So for full transparency @addyosmani, as I will disclose in the release post for Penthouse 1.0 (perhaps you know this already) - all my work on Penthouse is pretty much "funded" via the premium SASS service I built on top (https://criticalcss.com), which has been using image diffing since the start. It's also from where I've found most of the issues in Penthouse, and got the confidence that Penthouse works well, since it's used really heavily there. Will post another blog post about criticalcss.com too soon and my story about a successful open source revenue model for a "side project". :)

[pocketjoso]

And @addyosmani re sitting next to the Puppeteer team - that's awesome! You already found the most critical issue on my list (puppeteer/puppeteer#662).

I have nothing else critical that I've already opened issues for, but other things on my mind are:
- problems with thrown errors such as the ones making critical's tests fail - but I need to take a closer look my self on this first to see whether it's something I can fix in Penthouse. There are fewer such thrown errors since the commits after 0.10.2 (specifically since puppeteer/puppeteer@e5c17ee), but...

.. it also seems to me that there's been some performance regressions in Puppeteer since 0.10.2, at least how I use it (with JS disabled). On average my puppeteer execution now seems at least about 1s slower (from about 10s to 12s) - but I have not isolated the cause (exact commit) for this yet hence why I haven't raised an issue in Puppeteer yet. It is possible, but unlikely, that the regression did not come from a change in Puppeteer.

Update: after debugging this today I found and resolved these regressions on my end - they were not coming from Puppeteer. 👍

---

I also understand that Puppeteer have other priorities than making execution as fast as possible with JS disabled, so hey perhaps this is known and intentional. All in all Chrome headless is lightyears faster than PhantomJS anyway. :)

[pocketjoso]

Finally @addyosmani I have been thinking about backwards compatibility, and the return value of Penthouse, per the discussion we had in London a while ago. I considered breaking the api to return an object with both the critical css and debug/error info as you asked for back then, but for this release I'm almost sold on going for compatibility, so I can get maximum adaptation as the release is just so much better than the current version.

[pocketjoso]

Update: Just published penthouse@1.0.0-rc14 which fixes the bug with target closed errors as reported by @phaistonian in #229 (comment).

Doing final sanity testings, preparing to merge this into the Penthouse master branch for 1.0 version. However in addition to the release post I'm writing I also need to make sure Penthouse's README is up to date - so not sure I will manage to finish this tonight, let's see.

@addyosmani and @bezoerb - I added 2 more points re changes critical should take up when adopting this new penthouse version (no need to promisify, and should be able to skip starting local servers for local files): #229 (comment)

[reznord]

@pocketjoso everything works super awesome, except for this:

(node:5200) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Error: Penthouse timed out after 0.001s.
(node:5200) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[pocketjoso]

@reznord pushed one more commit that prevents rejecting the returned promise when Penthouse is called in callback mode - have not pushed this to npm though as I'm hoping to merge to master (next) instead :).

And in the bigger picture would be great for critical to start using the Promise based api instead.

[bezoerb]

@pocketjoso That's super awesome! Thanks for putting so much effort in this! I'm looking forward to make the suggested changes to critical this week.

[pocketjoso]

🎉 Penthouse 1.0 is here🎉:
https://medium.com/@pocketjoso/penthouse-1-0-official-release-ece995b0d29e

[bezoerb]

@pocketjoso Tests are currently failing for node 6.
appveyor,
travis

Seems like the current master of puppeteer needs an additional build step for node6 https://github.com/GoogleChrome/puppeteer/blob/master/package.json#L20
/cc @addyosmani

[pocketjoso]

@bezoerb yeah commented in Penthouse repo.

[phaistonian]

So? Almost there?:)

[bezoerb]

@phaistonian: Yes :) we only need to fix node6 support

[stereobooster]

Is there a PR to fix node6 tests?

[bezoerb]

one step forward in #246

[Grawl]

I found this Chrome extension: Critical CSS extractor

Works good for my sites.

[phaistonian]

How about a next tag until this is done?

i.e yarn add critical@next

[bezoerb]

v1.0.0 Released :)