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

feat(tailwind): Smaller bundle size #1383

Open
wants to merge 92 commits into
base: canary
Choose a base branch
from

Conversation

gabrielmfern
Copy link
Collaborator

@gabrielmfern gabrielmfern commented Mar 27, 2024

Background

After the updates to the Tailwind component that came with performance improvements
we had a lot more environments to consider while trying to run tailwindcss
under the hood for generating the styles from classes.

These environment constraints, like being able to run on the browser and on other environments,
forced us into having to polyfill certain node modules — that both postcss and tailwindcss
use under the hood — while bundling even though these code paths were never really
hit when running.

The reason these code paths were not completely tree shaken out of the built code was because
we called tailwindcss directly as a plugin on postcss.

Side note: This PR was first intended for #1104 but ended up into a better implementation of #1124

Why?

Polyfilling was the temporary solution for this, but it is not optimal in the least. This caused
issues like:

And ended up also being a burden to maintain and debug.

The solution

The best way I've found to solve these issues is by calling out the internal Tailwind functions
that do what they need to do on postcss data structures. This also allows us to substitute a bit of
our logic, which was previously purely done with regexes, with postcss's utilities which makes it much easier to read
and maintain.

The reason I choose to call out the internal Tailwind functions was because the code that was Node specific
was really just around finding the configuration or checking if a certain thing was installed, and
this would really only run when it was called as a postcss plugin.

The way I do call out the internal Tailwind functions is heavily inspired by the way they do it
themselves on the Tailwind VS Code extension/LSP Server.

Other changes that make this a refactor

Along with this, I also had to change up a few things. The first and biggest one was applying
the non-inlinable styles — currently media queries, i.e. sm:... — after our first pass
on the React tree which is for inlining styles. This is because we are now generating the styles
for each element's classes on-the-fly instead of at once (which is still performant the way it is
implemented), so we need to aggregate all the non-inlinable classes and their CSS to add to the <head>
later.

The second change should be mostly internal, (and should also make it easier for others to contribute),
which is a new mapReactTree utility that goes through the whole tree and separates the concerns
about when to go recursive on the elements or not. It will also make it easier to actually look at #1104 once
we get there.

Added tests for ensured stability

As I mentioned, we had to consider a bunch of different environments and testing them manually
was really time-consuming, so, borrowed from #1124, I implemented an integration tested powered by yalc that allows us to make sure that the component will work well with NextJS's build process.

You can find this test inside packages/tailwind/integrations/_tests/nextjs.spec.ts. This PR also adds a test for Vite as well.

@gabrielmfern gabrielmfern added Package: @react-email/tailwind Type: Improvement An improvement, that is, changes to an existing feature that improve it labels Mar 27, 2024
@gabrielmfern gabrielmfern self-assigned this Mar 27, 2024
Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-email ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 18, 2024 6:52pm
react-email-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 18, 2024 6:52pm

@@ -28,7 +28,6 @@
"pnpm": {
"patchedDependencies": {
"[email protected]": "patches/[email protected]",
"[email protected]": "patches/[email protected]",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This package was one that we used as a polyfill that isn't needed and isn't required to be patched anymore!

};

const nextAppLocation = path.resolve(__dirname, "./nextjs");
$("npm install", nextAppLocation);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried using pnpm here, which would be better for sure, but, for some reason, the preview deploy of our app at https://react.email failed because of it. Once I moved to using npm this worked without errors.

return processElement(renderedComponent);
if (hasNonInlineStylesToApply) {
let hasAppliedNonInlineStyles = false as boolean;
mappedChildren = mapReactTree(mappedChildren, (node) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Following the same idea from what we merged recently that allows the <head> element to be at any depth, this maps the entire React tree to find it.

@@ -22,6 +22,34 @@ describe("Tailwind component", () => {
});
});

// test("with React context and custom components", () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A unit test I kept commented out as useful for when we get to #1104.

endymion1818 and others added 20 commits March 28, 2024 08:07
Co-authored-by: Benny Burrito <[email protected]>
Co-authored-by: Ben Verspeak <[email protected]>
Co-authored-by: Gabriel Miranda <[email protected]>
This function is meant to do the sanitization that we need for the best email client support.
As of this commit, it only sanitizes the `rgb` calls it finds.
…name of their digit

This is because the only way to have these numbers in class names for CSS is to either escape them with something like "\3".
But the issue with that is that we can't really have escape characters in class selectors as they aren't very well supported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: @react-email/tailwind Type: Improvement An improvement, that is, changes to an existing feature that improve it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants