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

[Emotion] Add HOC util for memoized styles + memoize EuiIcon styles #7544

Merged
merged 4 commits into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 6 additions & 13 deletions src/components/icon/icon.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,9 @@
*/

import { css, keyframes } from '@emotion/react';
import { logicalCSS, euiCanAnimate } from '../../global_styling';
import { logicalSizeCSS, euiCanAnimate } from '../../global_styling';
import { UseEuiTheme } from '../../services';

const iconSize = (size: string) => {
return `
${logicalCSS('width', size)};
${logicalCSS('height', size)};
`;
};

export const iconLoadingOpacity = 0.05;

const iconLoading = keyframes`
Expand Down Expand Up @@ -100,11 +93,11 @@ export const euiIconStyles = ({ euiTheme }: UseEuiTheme) => ({
`,
// Sizes
original: css``,
s: css(iconSize(euiTheme.size.m)),
m: css(iconSize(euiTheme.size.base)),
l: css(iconSize(euiTheme.size.l)),
xl: css(iconSize(euiTheme.size.xl)),
xxl: css(iconSize(euiTheme.size.xxl)),
s: css(logicalSizeCSS(euiTheme.size.m)),
m: css(logicalSizeCSS(euiTheme.size.base)),
l: css(logicalSizeCSS(euiTheme.size.l)),
xl: css(logicalSizeCSS(euiTheme.size.xl)),
xxl: css(logicalSizeCSS(euiTheme.size.xxl)),
// Variants
// App icons are two-toned. This provides the base color.
app: css`
Expand Down
32 changes: 13 additions & 19 deletions src/components/icon/icon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import { enqueueStateChange } from '../../services/react';

import {
htmlIdGenerator,
withEuiTheme,
WithEuiThemeProps,
withEuiStylesMemoizer,
WithEuiStylesMemoizerProps,
} from '../../services';
export { COLORS } from './named_colors';
import { isNamedColor, NamedColor } from './named_colors';
Expand Down Expand Up @@ -130,11 +130,11 @@ export const appendIconComponentCache = (iconTypeToIconComponentMap: {
};

export class EuiIconClass extends PureComponent<
EuiIconProps & WithEuiThemeProps,
EuiIconProps & WithEuiStylesMemoizerProps,
State
> {
isMounted = false;
constructor(props: EuiIconProps & WithEuiThemeProps) {
constructor(props: EuiIconProps & WithEuiStylesMemoizerProps) {
super(props);

const { type } = props;
Expand Down Expand Up @@ -238,8 +238,8 @@ export class EuiIconClass extends PureComponent<
tabIndex,
title,
onIconLoad,
theme,
style,
stylesMemoizer,
...rest
} = this.props;

Expand Down Expand Up @@ -267,7 +267,7 @@ export class EuiIconClass extends PureComponent<
const classes = classNames('euiIcon', className);

// Emotion styles
const styles = euiIconStyles(theme);
const styles = stylesMemoizer(euiIconStyles);
const cssStyles = [
styles.euiIcon,
styles[size],
Expand Down Expand Up @@ -304,20 +304,14 @@ export class EuiIconClass extends PureComponent<
this.props['aria-labelledby'] ||
this.props.title
);
const hideIconEmpty = isAriaHidden && { 'aria-hidden': true };

let titleId: any;

// If no aria-label or aria-labelledby is provided but there's a title, a titleId is generated
// The svg aria-labelledby attribute gets this titleId
// The svg title element gets this titleId as an id
if (
!this.props['aria-label'] &&
!this.props['aria-labelledby'] &&
title
) {
titleId = { titleId: generateId() };
}
const titleId =
!this.props['aria-label'] && !this.props['aria-labelledby'] && title
? { titleId: generateId() }
: undefined;

return (
<Svg
Expand All @@ -327,16 +321,16 @@ export class EuiIconClass extends PureComponent<
tabIndex={tabIndex}
role="img"
title={title}
{...titleId}
data-icon-type={iconTitle}
data-is-loaded={isLoaded || undefined}
data-is-loading={isLoading || undefined}
{...titleId}
{...rest}
{...hideIconEmpty}
aria-hidden={isAriaHidden || undefined}
/>
);
}
}
}

export const EuiIcon = withEuiTheme<EuiIconProps>(EuiIconClass);
export const EuiIcon = withEuiStylesMemoizer<EuiIconProps>(EuiIconClass);
6 changes: 5 additions & 1 deletion src/services/theme/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ export {
} from './hooks';
export type { EuiThemeProviderProps } from './provider';
export { EuiThemeProvider } from './provider';
export { useEuiMemoizedStyles } from './style_memoization';
export {
useEuiMemoizedStyles,
withEuiStylesMemoizer,
type WithEuiStylesMemoizerProps,
} from './style_memoization';
export { getEuiDevProviderWarning, setEuiDevProviderWarning } from './warning';
export {
buildTheme,
Expand Down
68 changes: 67 additions & 1 deletion src/services/theme/style_memoization.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ import { testOnReactVersion } from '../../test/internal';
import type { UseEuiTheme } from './hooks';
import { EuiThemeProvider } from './provider';

import { useEuiMemoizedStyles } from './style_memoization';
import {
useEuiMemoizedStyles,
withEuiStylesMemoizer,
WithEuiStylesMemoizerProps,
} from './style_memoization';

describe('useEuiMemoizedStyles', () => {
beforeEach(jest.clearAllMocks);
Expand Down Expand Up @@ -83,3 +87,65 @@ describe('useEuiMemoizedStyles', () => {
}
);
});

describe('withEuiStylesMemoizer', () => {
beforeEach(jest.clearAllMocks);

const componentStyles = jest.fn(({ euiTheme }: UseEuiTheme) => ({
someComponent: css`
color: ${euiTheme.colors.dangerText};
`,
}));

class ComponentClass extends React.Component<WithEuiStylesMemoizerProps> {
state = {
rerender: false,
};

rerender = () => {
this.setState({ rerender: !this.state.rerender });
};

render() {
const styles = this.props.stylesMemoizer(componentStyles);
return <button css={styles.someComponent} onClick={this.rerender} />;
}
}
const Component = withEuiStylesMemoizer(ComponentClass);

it('memoizes the passed fn, only computing the styles once regardless of other rerenders', () => {
const { getByRole } = render(<Component />);

expect(componentStyles).toHaveBeenCalledTimes(1);
expect(getByRole('button')).toHaveStyleRule('color', '#bd271e');

fireEvent.click(getByRole('button'));
expect(componentStyles).toHaveBeenCalledTimes(1);
});

it('recomputes styles if the upstream theme changes', () => {
const { rerender, getByRole } = render(
<EuiThemeProvider colorMode="light">
<Component />
</EuiThemeProvider>
);
expect(componentStyles).toHaveBeenCalledTimes(1);
expect(getByRole('button')).toHaveStyleRule('color', '#bd271e');

rerender(
<EuiThemeProvider colorMode="dark">
<Component />
</EuiThemeProvider>
);
expect(componentStyles).toHaveBeenCalledTimes(2);
expect(getByRole('button')).toHaveStyleRule('color', '#f86b63');

// Should not recompute styles if no theme changes
rerender(
<EuiThemeProvider colorMode="dark">
<Component />
</EuiThemeProvider>
);
expect(componentStyles).toHaveBeenCalledTimes(2);
});
});
86 changes: 69 additions & 17 deletions src/services/theme/style_memoization.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import React, {
useContext,
useState,
useMemo,
useCallback,
forwardRef,
} from 'react';
import type { SerializedStyles, CSSObject } from '@emotion/react';

Expand Down Expand Up @@ -52,34 +54,84 @@ export const EuiThemeMemoizedStylesProvider: FunctionComponent<
);
};

/**
* Internal util primarily responsible for getting the memoized styles (if they exist)
* and if not, generating and setting the styles. DRYed out to facilitate usage
* between both hook/function components and HOC/class components
*/
const getMemoizedStyles = (
stylesGenerator: Function,
stylesMap: MemoizedStylesMap,
euiThemeContext: UseEuiTheme
) => {
if (!stylesGenerator.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.'
);
}
const existingStyles = stylesMap.get(stylesGenerator);
if (existingStyles) {
return existingStyles;
} else {
const generatedStyles = stylesGenerator(euiThemeContext);
stylesMap.set(stylesGenerator, generatedStyles);

return generatedStyles;
}
};

/**
* Hook that memoizes the returned values of components style fns/generators
* per-theme
*/
export const useEuiMemoizedStyles = <
T extends (theme: UseEuiTheme) => StylesMaps
>(
styleGenerator: T
stylesGenerator: T
): ReturnType<T> => {
const memoizedStyles = useContext(EuiThemeMemoizedStylesContext);
const euiThemeContext = useEuiTheme();

const memoizedComponentStyles = useMemo(() => {
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.'
);
}
const existingStyles = memoizedStyles.get(styleGenerator);
if (existingStyles) {
return existingStyles;
} else {
const generatedStyles = styleGenerator(euiThemeContext);
memoizedStyles.set(styleGenerator, generatedStyles);

return generatedStyles;
}
}, [styleGenerator, memoizedStyles, euiThemeContext]);
const memoizedComponentStyles = useMemo(
() => getMemoizedStyles(stylesGenerator, memoizedStyles, euiThemeContext),
[stylesGenerator, memoizedStyles, euiThemeContext]
);

return memoizedComponentStyles as ReturnType<T>;
};

/**
* HOC for class components
* Syntax is mostly copied from withEuiTheme HOC
*/
export interface WithEuiStylesMemoizerProps {
stylesMemoizer: typeof useEuiMemoizedStyles; // Type shortcut, we don't actually pass down the hook itself
}
export const withEuiStylesMemoizer = <T extends {} = {}>(
Component: React.ComponentType<T & WithEuiStylesMemoizerProps>
) => {
const componentName =
Component.displayName || Component.name || 'ComponentWithStylesMemoizer';
Copy link
Member

@tkajtoch tkajtoch Mar 5, 2024

Choose a reason for hiding this comment

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

This results in the HoC component using the same display name as the wrapped component, which is confusing in React DevTools and may seem like improperly nested icons.
Screenshot 2024-03-05 at 11 55 51

Could you please rename the HoC's displayName to match the recommended HoCName(WrappedComponentName) convention? In this case, that would be something like WithEuiStylesMemoizer(${Component.displayName || Component.name || 'Component'}). LMK what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copy of what we're already doing in withEuiTheme:

export const withEuiTheme = <T extends {} = {}, U extends {} = {}>(
Component: React.ComponentType<T & WithEuiThemeProps<U>>
) => {
const componentName =
Component.displayName || Component.name || 'ComponentWithTheme';
const Render = (
props: Omit<T, keyof WithEuiThemeProps<U>>,
ref: React.Ref<Omit<T, keyof WithEuiThemeProps<U>>>
) => {
const theme = useEuiTheme<U>();
return <Component theme={theme} ref={ref} {...(props as T)} />;
};
const WithEuiTheme = forwardRef(Render);
WithEuiTheme.displayName = componentName;
return WithEuiTheme;

I'm reluctant to change it for that reason, and also primarily because I believe it will massively fuck up snapshots (which was likely the original reason for Greg doing this weird shenanigans in the first place). I don't love it, but since we're already doing it and I don't really feel like going through a massively painful set of Kibana upgrades, I'd rather not change it.

Copy link
Member

Choose a reason for hiding this comment

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

I get it, but that's a new HoC, so enzyme snapshots will be affected anyway, won't they? DOM snapshots should all be fine since they aren't aware of React vDOM structure

Copy link
Contributor Author

@cee-chen cee-chen Mar 6, 2024

Choose a reason for hiding this comment

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

I get it, but that's a new HoC, so enzyme snapshots will be affected anyway, won't they?

No, they won't, because:

  1. The .displayName is overridden to use the original component displayName
  2. We're replacing withEuiTheme with withEuiStylesMemoizer - that's the whole point of keeping the override the same

React DevTools is not the same as Enzyme's shallow/mounted renderer and I believe this code was originally written with Enzyme in mind (bleh).

Copy link
Member

Choose a reason for hiding this comment

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

aah gotcha, I missed the withEuiTheme diff part! I'm good leaving it as is then and hope enzyme snapshots will comply 🤣


const Render = (
props: Omit<T, keyof WithEuiStylesMemoizerProps>,
ref: React.Ref<Omit<T, keyof WithEuiStylesMemoizerProps>>
) => {
const memoizedStyles = useContext(EuiThemeMemoizedStylesContext);
const euiThemeContext = useEuiTheme();
const stylesMemoizer = useCallback(
(stylesGenerator: Function) =>
getMemoizedStyles(stylesGenerator, memoizedStyles, euiThemeContext),
[memoizedStyles, euiThemeContext]
);
return (
<Component stylesMemoizer={stylesMemoizer} ref={ref} {...(props as T)} />
);
};

const WithEuiStylesMemoizer = forwardRef(Render);

WithEuiStylesMemoizer.displayName = componentName;

return WithEuiStylesMemoizer;
};
Loading