emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.43k stars 1.11k forks source link

@emotion/styled: Add Transient Props #2193

Open petermikitsh opened 3 years ago

petermikitsh commented 3 years ago

The problem

In 5.1.0, styled-components introduced transient props. Props prefixed with a dollar sign ($) are "mark[ed] as transient and styled-components knows not to add it to the rendered DOM element or pass it further down the component hierarchy".

Proposed solution

This would be useful functionality to support -- I am exploring migrating from styled-components to @emotion/styled.

Alternative solutions

None suggested. The intent is to follow this implementation detail introduced by another CSS-in-JS project.

Additional context

Material UI v4 -> v5 is migrating to emotion, and my project uses styled-components today. I'm trying to get my codebase to coalesce around a single CSS in JS solution, and the fewer the API differences there are, the lower the barrier to migration is.

akullpp commented 1 year ago

Anyone more knowledgeable about creating a wrapper with TS + MUI?

import { CreateStyled } from '@emotion/styled'
import { styled as _styled } from '@mui/material/styles'

const transientOptions: Parameters<CreateStyled>[1] = {
  shouldForwardProp: (propName: string) => {
    return !propName.startsWith('$')
  },
}

const styled = (element: Parameters<typeof _styled>[0]) => {
  return _styled(element, transientOptions)
}

export { styled }
ed-peretokin commented 6 months ago

Every solution on making a wrapper around the styled will be limited since TS will never know, how to process the overload or inferred generic types. I have made a solution, that will return us the original styled with using native Proxy functionality:

import { CreateMUIStyled, styled as _styled, Theme } from '@mui/material/styles';

const createProxyStyled = (): CreateMUIStyled<Theme> => {
  const handler: ProxyHandler<CreateMUIStyled<Theme>> = {
    apply: function (target, _, argArray) {
      return target(
        argArray[0],
        {
          ...(argArray[1] || {}),
          shouldForwardProp: (prop) =>
            !prop.toString().startsWith('$') &&
            (argArray[1]?.shouldForwardProp(prop) || true),
        }
      );
    },
  };

  return new Proxy(_styled, handler);
};

export const styled = createProxyStyled();
elektronik2k5 commented 6 months ago

Another possible workaround is monkey patching @emotion/is-prop-valid in your repo using patch-package (or the equivalent built-in feature in some package managers, like yarn). The patch would prepend prop.startsWith('$') && to what @emotion/is-prop-valid does. It is ugly, gnarly, fragile across version updates and requires non trivial knowledge, but it should work. (I haven't tried this approach cause I'm not facing this issue right now.)

I've been using and recommending Emotion for a long time and it is sad to see that when the greater ecosystem (MUI) embraces Emotion, the users' first experience is hitting this brick wall without an easy, simple and reliable workaround.

I understand and empathize with the maintainers' stance on API surface, scope and all that. Really, I do and I definitely support the notion that Emotion is not meant to be a drop-in replacement for styled-components, nor foster its design choices. However, it is also hard to ignore the fact that technically it should be a trivial issue to fix. @Andarist and @emmatown, if you don't wanna change the default, I suggest we come up with an easier escape hatch to make the migration smoother and allow easier customization - which also aligns well with Emotion's design goals (IIUC).

How about allowing overriding the default shouldForwardProp via a global configuration? For example, could we do it as part of the customizations which CacheProvider supports? Seems to me like the natural place for such an escape hatch.

Touseef-ahmad commented 3 months ago

I recently faced an issue, where I was getting warnings on the console, after hours of debugging I found the problem was that the props were being passed from my styled component to the Mui Typography component, which it not ideal, we would like to have a way to not pass the props through so we don't get these annoying warnings.

Screenshot 2024-05-30 at 11 03 15 AM
oigewan commented 3 months ago

I'd like to hear more about the specifics of the concerns (e.g., is there a weak or strong objection to the implementation detail). Thanks!

It's mostly around the fact that it creates a distinction between "styled components" and "regular components" with the naming scheme when whether something is a styled component or not should just be an implementation detail. We also try to maintain that styled(SomeStyledComponent) is practically equivalent to styled(forwardRef((props, ref) => <SomeStyledComponent {...props} ref={ref} />))(which iirc we do with the exception of withComponent) but you would likely want to make that not true for transient props(and it isn't with SC: https://codesandbox.io/s/muddy-surf-ihu6e) which we'd like to avoid.

@emmatown

I don't understand why allowing customization of the isPropValid function through context or something would break the pattern you describe. At least one prop is already filtered (theme), so filtering props which are internal implementation details of the lib is already an accepted pattern. We'd like a way to accomplish the same thing in a way that has a good DX. Seems quite unfortunate that this feature request is being ignored as there appears to be quite a bit of desire for it.