-
Notifications
You must be signed in to change notification settings - Fork 856
[POC] chore(eui): flag dynamic styles in emotion #8797
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
base: main
Are you sure you want to change the base?
[POC] chore(eui): flag dynamic styles in emotion #8797
Conversation
53c42fb
to
f2b6263
Compare
f2b6263
to
cc5a5a1
Compare
💚 Build SucceededHistory
|
💚 Build Succeeded
|
* Applies a monkey patch to the given Emotion cache to flag dynamic styles. | ||
* This is modular and can be reused in other places. | ||
*/ | ||
export function patchCacheForDynamicStyles(cache: EmotionCache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a good place to use the decorator pattern
* A utility to normalize selectors by removing the dynamic part | ||
* of the class name generated by Emotion. | ||
*/ | ||
function getBaseClass(selector: string): string | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that this implementation is configuration-sensitive and may break when a custom labelFormat
is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 The regex only works with the base cache setup, which is by default named as starting with "css". If a cache is created with createCache which requires a custom key (code) and that key is not "css" then this matching will not work.
We could consider exporting a set of cache names that are matched against to limit the risk. Or provide means to define the cache names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it is mentioned in one of the limitations in the description. The list of cache names could be limiting but is an idea we can explore.
if (hash) { | ||
hashes.add(hash); | ||
if (hashes.size > 1) { | ||
console.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning should likely be emitted only once per style to reduce console spam in cases of frequently updated styles (e.g., related to scroll positions).
flagDynamicStyles(selector, serialized); | ||
return originalInsert.call(this, selector, serialized, sheet, shouldCache); | ||
}; | ||
(cache as any).__patched = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this flag at all?
|
||
const panelStyles = (isPrimary: boolean) => { | ||
return css` | ||
background-color: ${isPrimary ? 'blue' : 'gray'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good example of a technically dynamic style that may or may not be harmful. If it changes too frequently, it might, but overall, this example is okay. It's more of a question of how we want users to define styles specific to component props (or other flags and settings). I usually prefer to follow BEM-like structure and treat props as modifiers to primary component styles, but that's not always handy - it all depends on the structure of the styles (e.g., if you need to style child classes you don't control, this might be your only solid solution).
Generally speaking, we can recommend that Kibana should use BEM-like modifier classes whenever dealing with props affecting component styles, but that's just internal. I wonder if it makes a lot of a difference considering the switch to speedy
being enabled by default there (and possibly becoming the default for EUI soon).
Do you know how does this detection work with Emotion's cssPropOptimization
?
[Slightly off-topic]
We need to benchmark if there's a difference between:
- A single style with dynamically changing value
- An array of styles which we compose based on props
- Using
style
the normal way - Using
style
orsetProperty
to pass a local CSS variable to use in the regular styles and letting browsers optimize it to reduce the number of style calculations and paints (I don't think this will be as performant as it sounds, though)
Without any data, I'd say that:
- A single style with a dynamically changing value will take some time to recompute all styles when any input value changes. This is usually very fast but could easily take 20ms for large stylesheets
- An array of styles could theoretically only recompute the stylesheet with just the single value (property) changing. This would be faster than recomputing the whole stylesheet, but it would also have to recreate the styles array and would be heavily dependent on other styles' caching which also has some resource cost associated with it
style
attribute updates will cause a rerender and repaint, similar toelement.style.setProperty
- Even if just a single class name changes, the element will need to have its styles recomputed and will likely cause a rerender. I'm not aware of any browser optimizations that would not rerender unless the new class names point to styles that are exactly the same
Note
This is strictly a PoC. It's not meant to be the final solution. It has to be thoroughly tested.
Summary
This PR monkey-patches the
cache.insert
function and keeps a map of unique selectors to determine whether styles are dynamic or not.Screen.Recording.2025-06-13.at.13.21.02.mov
Assumptions
css
prop or a style object).color
,size
, etc.), which is expected and not considered a dynamic style.css
,style
,color
) caused the style change; it only infers dynamic styles by observing multiple hashes for the same base class.style
prop for performance and maintainability.Advantages
style
prop for dynamic values),cache
, so no changes are required in component or app code.Disadvantages
css
vs.color
caused the style change - only that a dynamic style was generated - but could point to underlying issues in EUI as well,Feasibility
The approach is feasible and practical for most Emotion-based React projects. It leverages public APIs and common patterns. Can be added to a provider or setup file with minimal effort. Since it’s dev-only, there’s no impact on production builds or performance.
Limitations
css
prop usage and other sources of dynamic styles (e.g., dynamic theme, context, or computed props),.css-xxxxxx-baseClass
) and if this changes, the detection may fail,Why are we making this change?
As recommended by Emotion, you should use
style
for dynamic styles andcss
only for static styles.Impact to users
This is not a breaking change. It doesn't affect EUI visually.
It's strictly in development to help advocate the usage of
style
prop for dynamic styles.QA
Specific checklist
cache.insert
css
prop that changes reference on every render)