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.

Sign up for GitHub

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

Jump to bottom

feat: add ignored and excluded classes to critical CSS extraction in collect #1102

Merged
merged 3 commits into from
Dec 20, 2022
Merged

feat: add ignored and excluded classes to critical CSS extraction in collect #1102

merged 3 commits into from
Dec 20, 2022

Conversation

ezlie-nguyen
Copy link
Contributor

Motivation

Our codebase uses the postcss-rtlcss package to add .dir-ltr, .dir-rtl, or .dir prefixes to our CSS selectors, so that most of them are in the following format:

.dir-ltr.otherClassname {
   padding-bottom: 12px;
}

Since our HTML contains the dir-ltr class, when we use Linaria's collect function, a lot of unnecessary selectors are added to the critical CSS even though otherClassname is not present.

Also, we have media queries in this format:

@media(...) {
  .dir-ltr.otherClassname {
    padding-left: 12px;
  }
  .dir-rtl.otherClassname {
    padding-right: 12px;
  }
}

We want to specifically exclude the RTL selector when the direction is LTR and vice versa.

Summary

This PR proposes to add an opt-in ignoredClasses parameter to collect so that we can ignore dir-ltr, dir-rtl, and dir from the regex used to determine critical CSS styles when it is present in the HTML.

Also, the excludedClasses parameter will allow dir-rtl to be excluded, so that even if otherClassname is in the HTML, the .dir-rtl.otherClassname selector will not be critical.

Test plan

Tested with unit tests for the new parameters to the collect function.

@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: c56fd5f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@linaria/server Minor
linaria Patch
linaria-website Patch
esbuild-example Patch
rollup-example Patch
vite-example Patch
webpack5-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ezlie-nguyen ezlie-nguyen mentioned this pull request Oct 25, 2022
Closed
@ezlie-nguyen ezlie-nguyen marked this pull request as ready for review October 25, 2022 22:30
Anber
Anber requested changes Oct 26, 2022
View reviewed changes
Copy link
Collaborator

@Anber Anber left a comment

Choose a reason for hiding this comment

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

It looks good to me.
Could you please run pnpm changeset and describe your changes?

@ezlie-nguyen ezlie-nguyen changed the title Add ignored and excluded classes to critical CSS extraction in collect feat: add ignored and excluded classes to critical CSS extraction in collect Oct 26, 2022
@ezlie-nguyen
Copy link
Contributor Author

It looks good to me. Could you please run pnpm changeset and describe your changes?

@Anber added! Thanks for the review 🙂

lencioni
lencioni reviewed Nov 4, 2022
View reviewed changes
packages/server/src/collect.ts Outdated
Comment on lines 76 to 77
// eslint-disable-next-line no-plusplus
removedNodesFromOther++;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can avoid disabling eslint here like this:

Suggested change
// eslint-disable-next-line no-plusplus
removedNodesFromOther++;
removedNodesFromOther += 1;

packages/server/src/collect.ts Outdated
const animations = new Set();
const other = postcss.root();
const critical = postcss.root();
const stylesheet = postcss.parse(css);
const htmlClassesRegExp = extractClassesFromHtml(html);
const htmlClassesRegExp = extractClassesFromHtml(html, ignoredClasses);
const excludedClassesRegExp = new RegExp(excludedClasses.join('|'), 'gm');
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a good idea to escape the excludedClasses array for special regex characters.

packages/server/src/collect.ts Outdated
@@ -22,48 +25,68 @@ const extractClassesFromHtml = (html: string): RegExp => {
/\\|\^|\$|\{|\}|\[|\]|\(|\)|\.|\*|\+|\?|\|/g,
'\\$&'
);
htmlClasses.push(className);
if (className !== '' && !ignoredClasses.includes(className)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is run in a couple of loops, it might not be a bad idea to convert ignoredClasses to a Set outside of the loops for performance.

@ezlie-nguyen
Copy link
Contributor Author

@lencioni I think I addressed your comments, mind taking another look?

lencioni
lencioni reviewed Nov 7, 2022
View reviewed changes
packages/server/src/collect.ts Outdated
Comment on lines 13 to 20
function escapeRegex(string: string) {
return string.replace(/[-/\\^$*+?.()|[\]{}]/g, '\\$&');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about the regex here. Did you write this by hand or pull it from somewhere?

It might be worth borrowing lodash's implementation. https://github.com/lodash/lodash/blob/4.17.11/lodash.js#L14259-L14279

Here's their regex, which looks a little bit different than the one in this PR: https://github.com/lodash/lodash/blob/4.17.11/lodash.js#L152

Their code comment points to https://262.ecma-international.org/7.0/#sec-patterns as the data source for this list of characters (though that inline link didn't work for me maybe it is supposed to be this one? https://262.ecma-international.org/7.0/#sec-regular-expressions-patterns)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider also not using regex to do the string matching, and just doing string searching? (ie, indexOf)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite following what the string searching would look like? Won't we still have to call string.replace to sanitize it?

re: the regex, I pulled it from somewhere, but happy to replace it with the one from lodash.

jpnelson
jpnelson approved these changes Nov 7, 2022
View reviewed changes
packages/server/src/collect.ts Outdated
html: string,
css: string,
ignoredClasses: string[] = [],
excludedClasses: string[] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be an object of options, if we were to add more? ie

collect(
  html: string,
  css: string,
  { ignoredClasses, excludedClass }: { ignoredClasses?: string[], excludedClasses?: string[] }
)

packages/server/src/collect.ts Outdated
html: string,
css: string,
ignoredClasses: string[] = [],
excludedClasses: string[] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

For naming, to me, ignored and excluded are synonyms, so I have a hard time remembering which one is doing which. Is there some other name I'm not thinking of that would help clarify what these do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree the naming for this is tricky. What do you think about ignoredClasses and blockedClasses?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's about this thread? Are you going to change names or I can merge this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, I changed the param names and this should be good to go now!

@Anber Anber merged commit 77bcf2e into callstack:master Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpnelson jpnelson jpnelson approved these changes

@lencioni lencioni lencioni left review comments

@Anber Anber Anber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ezlie-nguyen @Anber @lencioni @jpnelson