Either refactor components to use Stencils or use shouldForwardProp to filter out props on styled components.

[mannycarrera4]

🐛 Bug Report

While we transition away from Emotion, some components use the styled API from Emotion and don't filter our props for styles that are conditional.

We should either audit all components that use style and make sure we use shouldForwardProp to filter our props, or move forward and use Stencils combined with modifiers to avoid this problem.

[NicholasBoll]

We're in a weird transition period some components use styled and others use Stencils. Sometimes we implicitly forward props to another component and sometimes we don't need to. For example, if we style another component, but don't add any new style props, we shouldn't filter anything out:

const MyStyledSelect = styled(Select)({
  // style overrides that don't add a new prop
})

If we use isValidProp, any prop that Select is expected to use to function will no longer be passed. It worked before because every component in CK used styled components which use isPropValid on all elements by default. This way an invalid prop was passed to components, but not elements. Now that we're not using styled for all styling, we need any component using styled to filter own it's own styled components. isPropValid is not the fastest function and often not appropriate. The most appropriate function is filtering out style props added by the styled component. For example:

const VisibilityComponet = styled(SomeOtherComponent, {
  shouldForwardProp: prop => prop !== 'visibility' // we added this, so we should filter it out
})<{visibility: boolean}>({
  // base styles
},
({visibility}) => {
  if (visibility) {
    return {} // visibility styles
  }
}
)

[wainokray-ho]

It seems like we have an override function of emotion's styled API at https://github.com/Workday/canvas-kit/blob/master/modules/react/common/lib/theming/styled.ts:

import {default as emotionStyled, CreateStyled, Interpolation, CSSObject} from '@emotion/styled';
import {CSSProperties} from '@workday/canvas-kit-react/tokens';

import {useTheme, EmotionCanvasTheme, ContentDirection} from './index';
import rtlCSSJS from 'rtl-css-js';

const noop = (styles: any) => styles;

// Pulled from https://github.com/emotion-js/emotion/blob/master/packages/styled-base/src/utils.js#L6 (not exported)
type Interpolations = Array<any>;
export type StyleRewriteFn = (obj?: CSSProperties) => CSSProperties | undefined;

function styled<Props>(node: any) {
  return (...args: Interpolation<Props>[]) => {
    const newArgs: Interpolations = args.map(
      interpolation => (
        props: Props & {theme: EmotionCanvasTheme & {_styleRewriteFn?: StyleRewriteFn}}
      ) => {
        props.theme = useTheme(props.theme);
        const direction = props.theme.canvas.direction;
        const maybeFlip = direction === ContentDirection.RTL ? rtlCSSJS : noop;
        const maybeConvert = props.theme._styleRewriteFn || noop;
        try {
          if (typeof interpolation === 'function') {
            return maybeFlip(maybeConvert(interpolation(props)) as CSSObject);
          }
          return maybeFlip(maybeConvert(interpolation) as CSSObject);
        } catch (e) {
          return maybeConvert(interpolation);
        }
      }
    );

    return emotionStyled(node)(newArgs);
  };
}

export default styled as CreateStyled;

shouldForwardPropsfails to filter out the specified props if the component uses this override styled function. However it works perfectly fine for components that use the base @emotion styled function though. I am following your example for using the shouldForwardProps function as well.

is this expected behavior?

[wainokray-ho]

Here's a PR that adds in capability to use shouldForwardProp on the overridden styled function and also add shouldForwardProp to all styled components that pass in style props as params: https://github.com/Workday/canvas-kit/pull/2819.