[Emotion] Memoize typography styles & mixins #7543
Conversation
- no one's using it - it isn't documented anywhere - we should likely try to remove unnecessary style exports where we can, and encourage using components directly instead
- not being used anywhere in EUI or KIbana, just inline them + move internal `_colorCSS` util to bottom of file
- just write out the style manually 🤷 honestly, it might need a redesign at some point anyway + clean up styles and how they're written
| return euiNumberFormat(euiTheme); | ||
| }; | ||
| export const useEuiNumberFormat = (): string => | ||
| useEuiMemoizedStyles<any>(euiNumberFormat); |
There was a problem hiding this comment.
I'm wondering if we could prevent using an any type here?
Could it make sense to enhance useEuiMemoizedStyles to return StylesMaps | string to support cases like useEuiNumberFormat? 🤔
type MemoizedStylesMap = WeakMap<Function, StylesMaps | string>;
const useEuiMemoizedStyles = <
T extends (theme: UseEuiTheme) => StylesMaps | string
>(
styleGenerator: T
): ReturnType<T> => {}
There was a problem hiding this comment.
I'm not sure how I feel about this. These utility functions return strings on purpose, and they should only be used in the css tagged template literal so that they can get processed, validated and injected in the runtime.
The only reason for them to be memoized is if they're passed directly to the css prop, not wrapped like this
css={css`
${useEuiNumberFormat()}
}`because in this case, when the component rerenders, the styles will be recalculated anyway and memoization won't help much.
The css prop doesn't support passing a raw style string even though the Interpolation type allows it.
I believe that memoization here is unnecessary, and the responsibility to memoize customer styles lies on the consumer. If our goal is to provide one-line emotion utilities that are also memoized, we should probably also export useEuiNumberFormatCss() that does that exact thing and is meant to be used when the only thing you need is that single style (e.g., <p css={useEuiNumberFormatCss()}></p>)
There was a problem hiding this comment.
I'm wondering if we could prevent using an any type here?
see my question here about where my Styles type was too restrictive 🥲 #7529 (comment)
In light of this conversation I think it was, and I should modify it.
These utility functions return strings on purpose
I don't know if I agree with that and I don't think our architecture was meant to be that deliberate to be honest with you. Some utilities return strings so they don't trigger additional appended Emotion classnames, some use the css template literal because they do want to insert additional classNames. TBH, I don't think Caroline really had a specific rhyme or reason for it.
Also, our Emotion useX hooks are also fairly arbitrary and not well planned out, they currently have zero usage outside of our own documentation and to be frank: I don't like the idea of maintaining them. I'd rather just get rid of them entirely if we have the option.
There was a problem hiding this comment.
Also, our Emotion useX hooks are also fairly arbitrary and not well planned out, they currently have zero usage outside of our own documentation and to be frank: I don't like the idea of maintaining them. I'd rather just get rid of them entirely if we have the option.
In that case, maybe it's not worth adding memoization to them since the outputs of these functions vary, and they aren't really used externally. There's no need for adding memoization to small and rarely used utility styles, especially if we aren't sure we want to keep them in the code base
There was a problem hiding this comment.
I'm fine with that, I'd also vote to just rip out useEuiNumberFormat in this PR / in our docs if no one objects. 🤷
There was a problem hiding this comment.
@tkajtoch is this PR good to merge on your end or do you have any further feedback/change requests?
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
|
Thanks y'all! |
Summary
This PR is a performance enhancement (+ opinionated removals of several unused CSS functions that were exported from component style files only and not as top-level APIs) on
EuiText,EuiTitle, andEuiLinkcomponents (+ typography mixins).No UI or classNames should have changed/regressed as part of this PR.
As always, I recommend code reviewing by commit.
QA
General checklist
- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Checked in mobile[ ] Added or updated jest and cypress tests[ ] A changelog entry exists and is marked appropriately.- Skipping the changelog on these PRs as they shouldn't affect either end-users or consumers