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

[system] Fix unnecessary styles created from sx #33752

Merged
merged 7 commits into from Aug 8, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/mui-styled-engine-sc/src/index.d.ts
Expand Up @@ -21,6 +21,11 @@ export { default as StyledEngineProvider } from './StyledEngineProvider';
export { default as GlobalStyles } from './GlobalStyles';
export * from './GlobalStyles';

/**
* For internal usage in `@mui/system` package
*/
export function processStyles(tag: React.ElementType, processor: (styles: any) => any): void;
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some prefix (internal_ or unstable_)?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point.


// These are the same as the ones in @mui/styled-engine
// CSS.PropertiesFallback are necessary so that we support spreading of the mixins. For example:
// '@font-face'?: Fontface | Fontface[]
Expand Down
11 changes: 11 additions & 0 deletions packages/mui-styled-engine-sc/src/index.js
Expand Up @@ -36,6 +36,17 @@ export default function styled(tag, options) {
return stylesFactory;
}

export const processStyles = (tag, processor) => {
// Styled-components attaches an instance to `componentStyle`.
// https://github.com/styled-components/styled-components/blob/da8151762dcf72735ffba358173d4c097f6d5888/packages/styled-components/src/models/StyledComponent.ts#L257
//
// The instance contains `rules` (the styles)
// https://github.com/styled-components/styled-components/blob/da8151762dcf72735ffba358173d4c097f6d5888/packages/styled-components/src/models/ComponentStyle.ts#L23
if (tag.componentStyle) {
tag.componentStyle.rules = processor(tag.componentStyle.rules);
}
};

export { ThemeContext, keyframes, css } from 'styled-components';
export { default as StyledEngineProvider } from './StyledEngineProvider';
export { default as GlobalStyles } from './GlobalStyles';
5 changes: 5 additions & 0 deletions packages/mui-styled-engine/src/index.d.ts
Expand Up @@ -11,6 +11,11 @@ export { default as StyledEngineProvider } from './StyledEngineProvider';
export { default as GlobalStyles } from './GlobalStyles';
export * from './GlobalStyles';

/**
* For internal usage in `@mui/system` package
*/
export function processStyles(tag: React.ElementType, processor: (styles: any) => any): void;

export interface SerializedStyles {
name: string;
styles: string;
Expand Down
9 changes: 9 additions & 0 deletions packages/mui-styled-engine/src/index.js
@@ -1,3 +1,4 @@
/* eslint-disable no-underscore-dangle */
import emStyled from '@emotion/styled';

export default function styled(tag, options) {
Expand Down Expand Up @@ -25,6 +26,14 @@ export default function styled(tag, options) {
return stylesFactory;
}

export const processStyles = (tag, processor) => {
// Emotion attaches all the styles as `__emotion_styles`.
// Ref: https://github.com/emotion-js/emotion/blob/16d971d0da229596d6bcc39d282ba9753c9ee7cf/packages/styled/src/base.js#L186
if (Array.isArray(tag.__emotion_styles)) {
tag.__emotion_styles = processor(tag.__emotion_styles);
}
};

export { ThemeContext, keyframes, css } from '@emotion/react';
export { default as StyledEngineProvider } from './StyledEngineProvider';
export { default as GlobalStyles } from './GlobalStyles';
20 changes: 13 additions & 7 deletions packages/mui-system/src/createStyled.js
@@ -1,4 +1,5 @@
import styledEngineStyled from '@mui/styled-engine';
/* eslint-disable no-underscore-dangle */
import styledEngineStyled, { processStyles } from '@mui/styled-engine';
import { getDisplayName } from '@mui/utils';
import createTheme from './createTheme';
import propsToClassKey from './propsToClassKey';
Expand Down Expand Up @@ -82,7 +83,17 @@ export default function createStyled(input = {}) {
slotShouldForwardProp = shouldForwardProp,
styleFunctionSx = defaultStyleFunctionSx,
} = input;

const systemSx = (props) => {
const theme = isEmpty(props.theme) ? defaultTheme : props.theme;
return styleFunctionSx({ ...props, theme });
};
systemSx.__mui_systemSx = true;

return (tag, inputOptions = {}) => {
// Filter out the `sx` style function from the previous styled component to prevent unnecessary styles generated by the composite components.
processStyles(tag, (styles) => styles.filter((style) => !style?.__mui_systemSx));

const {
name: componentName,
slot: componentSlot,
Expand Down Expand Up @@ -131,7 +142,6 @@ export default function createStyled(input = {}) {
// On the server Emotion doesn't use React.forwardRef for creating components, so the created
// component stays as a function. This condition makes sure that we do not interpolate functions
// which are basically components used as a selectors.
// eslint-disable-next-line no-underscore-dangle
return typeof stylesArg === 'function' && stylesArg.__emotion_real !== stylesArg
? ({ theme: themeInput, ...other }) => {
return stylesArg({
Expand Down Expand Up @@ -176,10 +186,7 @@ export default function createStyled(input = {}) {
}

if (!skipSx) {
expressionsWithDefaultTheme.push((props) => {
const theme = isEmpty(props.theme) ? defaultTheme : props.theme;
return styleFunctionSx({ ...props, theme });
});
expressionsWithDefaultTheme.push(systemSx);
}

const numOfCustomFnsApplied = expressionsWithDefaultTheme.length - expressions.length;
Expand All @@ -194,7 +201,6 @@ export default function createStyled(input = {}) {
// On the server Emotion doesn't use React.forwardRef for creating components, so the created
// component stays as a function. This condition makes sure that we do not interpolate functions
// which are basically components used as a selectors.
// eslint-disable-next-line no-underscore-dangle
styleArg.__emotion_real !== styleArg
) {
// If the type is function, we need to define the default theme.
Expand Down
39 changes: 39 additions & 0 deletions packages/mui-system/src/createStyled.test.js
@@ -1,5 +1,6 @@
import * as React from 'react';
import { expect } from 'chai';
import { spy } from 'sinon';
import { ThemeProvider, createTheme } from '@mui/material/styles';
import { createRenderer } from 'test/utils';
import createStyled from './createStyled';
Expand Down Expand Up @@ -51,6 +52,44 @@ describe('createStyled', () => {
});
});

describe('composition', () => {
it('should call styleFunctionSx once', () => {
const styled = createStyled();
const spySx = spy();
const Child = styled('div')({});

render(<Child sx={spySx} />);

expect(spySx.callCount).to.equal(2); // React 18 renders twice in strict mode.
});

it('should still call styleFunctionSx once', () => {
const styled = createStyled();
const spySx = spy();
const Child = styled('div')({});
const Parent = styled(Child)({});

render(<Parent sx={spySx} />);

expect(spySx.callCount).to.equal(2); // React 18 renders twice in strict mode.
});

it('both child and parent still accept `sx` prop', () => {
const styled = createStyled();
const Child = styled('div')({});
const Parent = styled(Child)({});

const { container } = render(
<React.Fragment>
<Parent sx={{ color: 'rgb(0, 0, 255)' }} />
<Child sx={{ color: 'rgb(255, 0, 0)' }} />
</React.Fragment>,
);
expect(container.firstChild).toHaveComputedStyle({ color: 'rgb(0, 0, 255)' });
expect(container.lastChild).toHaveComputedStyle({ color: 'rgb(255, 0, 0)' });
});
});

describe('styles', () => {
it('styles of pseudo classes of variants are merged', () => {
const theme = createTheme({
Expand Down