Add forceInclude option for :where selectors
#11
Conversation
For some reason, penthouse doesn't think that :where selectors are not part of the critical css. Let's add it to the config so that it includes the :where selectors in the generated critical css. See iFixit/ifixit#47766 (comment) for more info.
critical-css.js
Outdated
| @@ -46,6 +46,7 @@ function findCriticalCss(cssString, url) { | |||
| height: 2160, | |||
| blockJSRequests: false, | |||
| forceExclude: excludeArr, | |||
| forceInclude: [/where/i], | |||
There was a problem hiding this comment.
As I understand it, this is going to force including any CSS selector that matches. I think we want to be as specific as we can be here because I don't think it's necessarily the case that every use of :where() is critical CSS. Beyond that, I think /where/ is too broad as that would match selectors like .where-empty.
There was a problem hiding this comment.
I think we want to be as specific as we can be here because I don't think it's necessarily the case that every use of :where() is critical CSS.
I agree. In the future, if we want to exclude some of the :where() uses, then we can exclude them by specifically adding to this array in our exclude array in critical.css.json config like the following: "excludes": [":where(.someSpecificSelector)"]
Beyond that, I think /where/ is too broad as that would match selectors like .where-empty.
I modified the regex to match other css features as requested in https://github.com/iFixit/ifixit/pull/47766#issuecomment-1544475631. Updated in 80656ab.
The new regex will match selectors like the following: :where(), :has(), :not(), :host(), :root().
Verified that it will not match selectors like the following: li(:last-child), .green:not(:disabled), and .card-hover:hover.
There was a problem hiding this comment.
I'm still not convinced that this is an appropriate solution. The point of the tool is that it identifies which CSS ultimately affects above-the-fold content. We're unconditionally insisting that any of these selectors always affects above-the-fold content, which is surely false. While that may be a solution to the problem we were seeing, I think at best it's papering over some deeper issue, either with penthouse or css-gather or our styles in general.
If this fixes the problem we were seeing (CLS), then as a temporary measure I think we could add more-specific forceInclude selectors that, when omitted, cause the CLS that we're currently seeing. But as-is, this pull has the potential to add a bunch of CSS that isn't critical to our critical CSS, and that means sending more bytes than we need to on every page load because we send that CSS directly in the HTML.
There was a problem hiding this comment.
It's not optimal, but our options are limited. Your points are all valid, but without these changes, the resets are removed entirely. That would cause major CLS on every page.
I looked at the penthouse repo and the last closed issue was about a year ago. I doubt the maintainer is still active. Should we spin up an issue to address this more thoroughly? I'd be happy to contribute here if it would help.
There was a problem hiding this comment.
They are on a super old version of puppeteer. Might be related.
There was a problem hiding this comment.
I'm ok with being explicit, but we are going to have to be careful moving forward if the reset ever changes. I was hoping to avoid manual work (hence the regex), but I don't see a median in this situation. Perhaps we can put a note in the automated PR description to check if there have been any reset changes since the previous week's run? It rarely changes, so that may be overkill.
There was a problem hiding this comment.
I was hoping to avoid manual work (hence the regex), but I don't see a median in this situation.
Totally fair. I'd just like us to prove that we're going to need more manual work before we try to solve for that. I'm hoping that we figure out a way to address the root cause before that becomes a problem. I think that @mlahargou's theory about being on an old version of puppeteer is a promising avenue to pursue. Ideally that gets fixed upstream, but perhaps we just need to fork and apply the pull linked in that comment.
There was a problem hiding this comment.
I just verified that pocketjoso/penthouse#347 fixes the problem.
Manually updated the penthouse in package.json to point at the fork (with the updated puppeteer version) from the comment and re-ran the css-gather. The generated critical-css now includes the :where selectors.
"cssnano-preset-advanced": "^5.0.0",
- "penthouse": "^2.3.2",
+ "penthouse": "feedanal/penthouse#master",
"postcss": "^8.2.10",In that case, should we just close this pull and make a fork for penthouse? Then when pocketjoso/penthouse#347 gets fixed we can drop the fork. That seems like a better idea.
There was a problem hiding this comment.
Going to close this pull, create a fork for penthouse with updated puppeteer, and use that fork for css-gather. 👍
This new regex will match selectors like the following: :where(), :has(), :not(), :host(), :root(). Verified that it will not match selectors like the following: li(:last-child), .green:not(:disabled), .card-hover:hover.
|
Steps used to test:
QA 🚛 |
Summary
For some reason, penthouse doesn't think that
:whereselectors are not part of the critical css.Let's add it to the config so that it includes the
:whereselectors in the generated critical css.Edit: The new regex will match selectors like the following:
:where(),:has(),:not(),:host(),:root().See https://github.com/iFixit/ifixit/pull/47766#issuecomment-1544460438 for more info.
QA notes
We can qa this on
iFixit/ifixitside by running thecritical.cssworkflow (with this branch checked out).cc: @ianrohde @mlahargou