[Emotion] Create hook utility for memoizing component styles per-theme #7529

cee-chen · 2024-02-14T17:06:55Z

Summary

⚠️ As always, I strongly recommend reviewing by commit.

This PR is an initial setup PR for memoizing a component's Emotion styles per theme (instead of per component). This should (hopefully) greatly improve the performance of our Emotion styles/object generation, especially for components used repeatedly in many places, e.g. EuiCode, EuiButton, etc.

I replaced the memoization used in #7486 with this new architecture as a proof of concept; this PR should maintain the performance improvements of the linked PR while being easy to swap in and replace across all of EUI's existing components, e.g.

const euiTheme = useEuiTheme();
const styles = euiComponentStyles(euiTheme);

// becomes

const styles = useEuiMemoizedStyles(euiComponentStyles);

What this PR does not do

Handle class components - I have a spike for this in a separate branch, but would prefer to keep this PR small for now to focus on the overlying idea/concept
Memoize dark vs. light themes - I played around with trying to memoize both light and dark themes separately, to make swapping back and forth faster/more performant, but eventually gave up on the idea as the full theme reference re-instantiates every time colorMode changes (which makes sense as theme tokens are also changing). TBH I also anticipate this as not being necessary as most end-users will not be swapping back and forth frequently between light/dark modes
Convert all components to use this util - this is a very basic setup and proof of concept of 2 components. If it passes muster, I'll open more follow-up PRs in batches later.

QA

EuiBeacon CSS rendered/looks the same as before as before
EuiCode CSS renders the same as before
EuiCode Performance
- Open your devtools console
- euiCodeSyntaxVariables() only fires 3 times on page load
- euiCodeSyntaxVariables() does not fire on fullscreen mode state change nor on page navigation
- euiCodeSyntaxVariables() does fire again on dark/light screen mode change (because the theme variables have changed)

General checklist

Revert [REVERT ME] commit
Browser QA
- Checked in both light and dark modes
- Checked in Chrome, Safari, Edge, and Firefox
  ~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
  - [ ] Checked in mobile
Docs site QA - N/A, this is mostly an internal EUI utility
Code quality checklist
- Added or updated jest ~~and cypress tests~~
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist - N/A

- replace previous `UseEuiCodeSyntaxVariables` memoization with this new architecture

…ting map entries every rerender

cee-chen · 2024-02-14T17:53:44Z

src/services/theme/style_memoization.tsx

+ * Hook that memoizes the returned values of components style fns/generators
+ * per-theme
+ */
+export const useEuiMemoizedStyles = <


[Feedback request] I am not super in love with this name or anything and am very open to renaming it something snappier! (applies to the general context and file name as well)

just useEuiStyles imo

@kqualters-elastic I'm going to stick with the useEuiMemoizedStyles name for now (it feels a little weird to drop Memoized as it's the entire point of the utility). But if we can always change it later down the road if it gets tedious! It's currently a mostly internal/beta utility at this point in any case 🙏

cee-chen · 2024-02-14T18:00:05Z

src/services/theme/style_memoization.tsx

+    if (!styleGenerator.name) {
+      throw new Error(
+        'Styles are memoized per function. Your style functions must be statically defined in order to not create a new map entry every rerender.'
+      );
+    }


This catch exists to attempt to catch anonymous functions that re-instantiate every rerender, which end up adding potentially hundreds of map entries that we don't need/want instead of a single one. Style generator functions should ideally be static constants to correctly memoize, since I'm using the function itself as the key to its mapped output.

I'd be interested in hearing people's thoughts as to whether that's a spicy decision. In theory I could use function.name as the map key instead, but I think that leads to potentially worse end-user behavior (what if we use the same generic function name, e.g. styles() in two places, and the wrong styles get mapped/sent back?).

Would be down to hear more thoughts about which approach feels safer either from a 'things break' perspective or 'going OOM' perspective 😅

We should definitely keep the function reference as the key, so there's no easy way for them to collide!

Quick note on the above: Tomasz's suggestion to change from Map() to WeakMap() (chatted about in Slack) should also help with issues where named functions get regenerated every rerender. 875dd01

cee-chen · 2024-02-14T18:01:33Z

src/services/theme/style_memoization.tsx

+type Styles = SerializedStyles | CSSObject | string | null; // Is this too restrictive?
+type StylesMaps = Record<string, Styles | Record<string, Styles>>;
+type MemoizedStylesMap = Map<Function, StylesMaps>;


This is me trying to anticipate/type out the general shape that our component styles objects take, but I'm not sure if it's too restrictive honestly and if I should just do Map<Function, any> 🤷

It looks good to me as is. We can always make it less restrictive later without breaking compatibility

tkajtoch

Tested locally and confirmed the performance benefits, especially when scaled to multiples of the same components per page. The code looks clean, and the implementation should scale well.

- to allow browser GC / avoid possible edge cases with massive RAM usage

- it's more verbose but oh well

This reverts commit cd77f3a.

kibanamachine · 2024-02-15T23:16:03Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7529/
Storybook: https://eui.elastic.co/pr_7529/storybook

elasticmachine · 2024-02-15T23:16:49Z

💚 Build Succeeded

Buildkite Build
Commit: 7864234

History

💚 Build #1409 succeeded cd77f3a

cc @cee-chen

`v93.1.1`⏩ `v93.2.0` --- - Updated `EuiPageSidebar` and `EuiPageTemplate.Sidebar` with a new `hasEmbellish` prop (defaults to false) ([#7521](elastic/eui#7521)) - Added `diff` glyph to `EuiIcon` ([#7520](elastic/eui#7520)) - Added `newChat` glyph to `EuiIcon` ([#7524](elastic/eui#7524)) **Bug fixes** - Fixed `EuiSideNav` not correctly typing the `items` prop as required ([#7521](elastic/eui#7521)) - Fixed the `CSS is not defined` bug in `EuiPageTemplate` when rendering in some SSR environments, particularly Next.js v13 and up ([#7525](elastic/eui#7525)) - Fixed `EuiDataGrid` component to clean up timer from side effect on unmount ([#7534](elastic/eui#7534)) **Accessibility** - Fixed `EuiSideNav` to render a fallback aria-label on mobile toggles if no heading or mobile title exists ([#7521](elastic/eui#7521)) **CSS-in-JS conversions** - Converted `EuiSideNav` to Emotion; Removed the following Sass variables: ([#7521](elastic/eui#7521)) - `$euiSideNavEmphasizedBackgroundColor` - `$euiSideNavRootTextcolor` - `$euiSideNavBranchTextcolor` - `$euiSideNavSelectedTextcolor` - `$euiSideNavDisabledTextcolor` - Removed the `euiSideNavEmbellish` Sass mixin. Use the new `EuiPageSidebar` `hasEmbellish` prop instead ([#7521](elastic/eui#7521)) - Added a new memoization/performance optimization utility for CSS-in-JS styles ([#7529](elastic/eui#7529))

`v93.1.1`⏩ `v93.2.0` --- - Updated `EuiPageSidebar` and `EuiPageTemplate.Sidebar` with a new `hasEmbellish` prop (defaults to false) ([elastic#7521](elastic/eui#7521)) - Added `diff` glyph to `EuiIcon` ([elastic#7520](elastic/eui#7520)) - Added `newChat` glyph to `EuiIcon` ([elastic#7524](elastic/eui#7524)) **Bug fixes** - Fixed `EuiSideNav` not correctly typing the `items` prop as required ([elastic#7521](elastic/eui#7521)) - Fixed the `CSS is not defined` bug in `EuiPageTemplate` when rendering in some SSR environments, particularly Next.js v13 and up ([elastic#7525](elastic/eui#7525)) - Fixed `EuiDataGrid` component to clean up timer from side effect on unmount ([elastic#7534](elastic/eui#7534)) **Accessibility** - Fixed `EuiSideNav` to render a fallback aria-label on mobile toggles if no heading or mobile title exists ([elastic#7521](elastic/eui#7521)) **CSS-in-JS conversions** - Converted `EuiSideNav` to Emotion; Removed the following Sass variables: ([elastic#7521](elastic/eui#7521)) - `$euiSideNavEmphasizedBackgroundColor` - `$euiSideNavRootTextcolor` - `$euiSideNavBranchTextcolor` - `$euiSideNavSelectedTextcolor` - `$euiSideNavDisabledTextcolor` - Removed the `euiSideNavEmbellish` Sass mixin. Use the new `EuiPageSidebar` `hasEmbellish` prop instead ([elastic#7521](elastic/eui#7521)) - Added a new memoization/performance optimization utility for CSS-in-JS styles ([elastic#7529](elastic/eui#7529))

cee-chen added 3 commits February 14, 2024 09:01

Set up memoized styles context

c48d9d7

Test basic usage

001f7f1

Convert more complex EuiCode

3c4fdee

- replace previous `UseEuiCodeSyntaxVariables` memoization with this new architecture

cee-chen added emotion performance labels Feb 14, 2024

cee-chen changed the title ~~[Emotion] Create utility for memoizing component styles per-theme~~ [Emotion] Create hook utility for memoizing component styles per-theme Feb 14, 2024

cee-chen added 3 commits February 14, 2024 09:43

Add a basic catch and warning for anonymous/non-static functions crea…

2c95aec

…ting map entries every rerender

changelog

6051740

[REVERT ME] console.log to show number of code style calls

cd77f3a

cee-chen force-pushed the emotion/per-theme-style-memoization branch from 4020fb1 to cd77f3a Compare February 14, 2024 17:43

cee-chen marked this pull request as ready for review February 14, 2024 17:52

cee-chen requested a review from a team as a code owner February 14, 2024 17:52

cee-chen commented Feb 14, 2024

View reviewed changes

cee-chen self-assigned this Feb 14, 2024

tkajtoch self-requested a review February 15, 2024 11:19

tkajtoch approved these changes Feb 15, 2024

View reviewed changes

cee-chen added 3 commits February 15, 2024 14:46

[PR feedback] Switch from Map to WeakMap

875dd01

- to allow browser GC / avoid possible edge cases with massive RAM usage

[misc naming] fix provider name to match context

a039649

- it's more verbose but oh well

Revert "[REVERT ME] console.log to show number of code style calls"

7864234

This reverts commit cd77f3a.

cee-chen enabled auto-merge (squash) February 15, 2024 22:53

cee-chen merged commit 47ab766 into elastic:main Feb 15, 2024
7 checks passed

cee-chen deleted the emotion/per-theme-style-memoization branch February 15, 2024 23:16

cee-chen mentioned this pull request Feb 20, 2024

Upgrade EUI to v93.2.0 elastic/kibana#177361

Merged

This was referenced Mar 6, 2024

[Emotion] Memoize typography styles & mixins #7543

Merged

[Emotion] Memoize all generated styles #7561

Open

cee-chen mentioned this pull request Mar 27, 2024

[Emotion] Allow configuring style memoization error/warning level #7626

Merged

5 tasks

cee-chen mentioned this pull request Aug 6, 2024

[Emotion][microperf] Separate form max-width variable to its own utility #7948

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Create hook utility for memoizing component styles per-theme #7529

[Emotion] Create hook utility for memoizing component styles per-theme #7529

cee-chen commented Feb 14, 2024 •

edited

Loading

cee-chen Feb 14, 2024

kqualters-elastic Feb 15, 2024

cee-chen Feb 15, 2024 •

edited

Loading

cee-chen Feb 15, 2024

cee-chen Feb 14, 2024 •

edited

Loading

tkajtoch Feb 15, 2024

cee-chen Feb 15, 2024

cee-chen Feb 14, 2024

tkajtoch Feb 15, 2024

tkajtoch left a comment

kibanamachine commented Feb 15, 2024

elasticmachine commented Feb 15, 2024

[Emotion] Create hook utility for memoizing component styles per-theme #7529

[Emotion] Create hook utility for memoizing component styles per-theme #7529

Conversation

cee-chen commented Feb 14, 2024 • edited Loading

Summary

What this PR does not do

QA

General checklist

cee-chen Feb 14, 2024

Choose a reason for hiding this comment

kqualters-elastic Feb 15, 2024

Choose a reason for hiding this comment

cee-chen Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

cee-chen Feb 15, 2024

Choose a reason for hiding this comment

cee-chen Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

tkajtoch Feb 15, 2024

Choose a reason for hiding this comment

cee-chen Feb 15, 2024

Choose a reason for hiding this comment

cee-chen Feb 14, 2024

Choose a reason for hiding this comment

tkajtoch Feb 15, 2024

Choose a reason for hiding this comment

tkajtoch left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 15, 2024

elasticmachine commented Feb 15, 2024

💚 Build Succeeded

History

cee-chen commented Feb 14, 2024 •

edited

Loading

cee-chen Feb 15, 2024 •

edited

Loading

cee-chen Feb 14, 2024 •

edited

Loading