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/styled: Add Transient Props #2193

Open
petermikitsh opened this issue Dec 25, 2020 · 57 comments
Open

@emotion/styled: Add Transient Props #2193

petermikitsh opened this issue Dec 25, 2020 · 57 comments

Comments

@petermikitsh
Copy link

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.

@Andarist
Copy link
Member

I'm not super keen about it but we also maintain compatibility with SC (unless we feel strongly that it's not worth it) so it might be worth adding support for this. @mitchellhamilton thoughts?

@emmatown
Copy link
Member

emmatown commented Jan 5, 2021

I'm also not super keen. For migration purposes, you could have a wrapper around styled that implements it with shouldForwardProp.

we also maintain compatibility with SC (unless we feel strongly that it's not worth it) so it might be worth adding support for this

This isn't totally true. We don't have .attrs, .withConfig, createGlobalStyle, the as prop is forwarded by default for non-dom element types, css is eagerly evaluated and there are probably some other subtle differences. I think that anything you can do in s-c, there should be an equivalent way to do in Emotion, if it's not the exact same API though, that's fine.

@petermikitsh
Copy link
Author

More context on the rationale for why the $-prefix can be useful (emphasis added):

Think of transient props as a lightweight, but complementary API to shouldForwardProp. Because styled-components allows any kind of prop to be used for styling (a trait shared by most CSS-in-JS libraries, but not the third party library ecosystem in general), adding a filter for every possible prop you might use can get cumbersome.

Transient props are a new pattern to pass props that are explicitly consumed only by styled components and are not meant to be passed down to deeper component layers. Here's how you use them:

 const Comp = styled.div`
   color: ${props => props.$fg || 'black'};
 `;

 render(<Comp $fg="red">I'm red!</Comp>);

It's presented primarily as quality-of-life, developer-experience type improvement (though I'd imagine the value of it is certainly subjective).

Additionally, styled components discusses forwarding more here (I believe this part aligns with Emotion):

If the styled target is a simple element (e.g. styled.div), styled-components passes through any known HTML attribute to the DOM. If it is a custom React component (e.g. styled(MyComponent)), styled-components passes through all props.

https://styled-components.com/docs/basics#passed-props

Emotion's current prop-forwarding is summarized as:

By default, Emotion passes all props (except for theme) to custom components and only props that are valid html attributes for string tags. You can customize this by passing a custom shouldForwardProp function. You can also use @emotion/is-prop-valid (which is used by emotion internally) to filter out props that are not valid as html attributes.

https://emotion.sh/docs/styled#customizing-prop-forwarding

The suggested workaround is a wrapper, which could look roughly something like this (if the thinking is incorrect here, please feel free to elaborate):

import styled from '@emotion/styled';

export const StyledEmotionInterop = (component: string | React.Component, template: string) => {
  return styled(component, {
    shouldForwardProp: (prop: string) => !prop.startsWith('$')
  })(template);
}

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!

@jmca
Copy link

jmca commented Jan 23, 2021

One downfall of using a wrapper function:
When the styled tag is picked up by Emotion's babel autoLabel, the [local] name generated will be the name of the wrapper function (StyledEmotionInterop in the above case), and not the component defined.

@jmca
Copy link

jmca commented Jan 23, 2021

This is the pattern I have been using successfully with Material UI:

export const transientOptions: Parameters<CreateStyled>[1] = {
  shouldForwardProp: (propName: string) => !propName.startsWith('$'),
};
const SomeComponent = styled(
  Typography,
  transientOptions,
)<{ $customProp: boolean }>(({ theme, $customProp }) => ({...}));

Note that the shouldForwardProp will apply to components that extend the root component:

const AnotherComponent = styled(
  SomeComponent  /* no need to pass transientOptions again */
)<{ $customProp2: boolean }>(({ theme, $customProp2 }) => ({...}));

The downfall is that it is opt-in (you have to explicitly use transientOptions), at least on the root component.

@Andarist
Copy link
Member

When the styled tag is picked up by Emotion's babel autoLabel, the [local] name generated will be the name of the wrapper function (StyledEmotionInterop in the above case), and not the component defined.

I'm not sure if that's totally correct - you should be able to utilize importMap to make labels behave pretty much the same as with the "builtin" styled.

@jmca
Copy link

jmca commented Jan 23, 2021

Oh nice. I actually didn't even know that existed. Guess I should look at those docs more 😅

@jmca
Copy link

jmca commented Jan 23, 2021

@Andarist Could you give an example of importMap's usage? The docs are a bit brief.

@Andarist
Copy link
Member

importMap: {
'package-one': {
nonDefaultStyled: {
canonicalImport: ['@emotion/styled', 'default']
}
},
'package-two': {
someJsx: {
canonicalImport: ['@emotion/react', 'jsx']
},
someCssFromCore: {
canonicalImport: ['@emotion/react', 'css']
},
SomeGlobalFromCore: {
canonicalImport: ['@emotion/react', 'Global']
}
},
'package-three': {
something: {
canonicalImport: ['@emotion/css', 'css']
}
},
'package-four': {
nonDefaultStyled: {
canonicalImport: ['@emotion/styled', 'default'],
styledBaseImport: ['package-four/base', 'something']
}
}
}

@jmca
Copy link

jmca commented Jan 24, 2021

@Andarist How does this apply to relative imports within the same codebase?

Say a file in the same codebase defines and exports a styled wrapper function.
Then another file (in the same codebase) imports that file via relative import path.
How would one configure importMap to make sure that wrapper function doesn't "steal" the auto-label?

@Andarist
Copy link
Member

Well - I would recommend creating a package for your own styled wrapper so you could configure a single entry for the importMap. Relative paths makes things worse - although you could copy the same thing with different paths leading to your styled from your app..

How would one configure importMap to make sure that wrapper function doesn't "steal" the auto-label?

I don't think it should steal it - but it might add an unnecessary label in this case. You could configure Babel to ignore the file with styled wrapper so it wouldn't happen. Wrapping in a package would also make it easier.

@jmca
Copy link

jmca commented Jan 24, 2021

@Andarist Thanks for the suggestion. Too bad importMap doesn't support regex.

@emmatown
Copy link
Member

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.

@sauldeleon
Copy link

This is the pattern I have been using successfully with Material UI:

export const transientOptions: Parameters<CreateStyled>[1] = {
  shouldForwardProp: (propName: string) => !propName.startsWith('$'),
};
const SomeComponent = styled(
  Typography,
  transientOptions,
)<{ $customProp: boolean }>(({ theme, $customProp }) => ({...}));

Note that the shouldForwardProp will apply to components that extend the root component:

const AnotherComponent = styled(
  SomeComponent  /* no need to pass transientOptions again */
)<{ $customProp2: boolean }>(({ theme, $customProp2 }) => ({...}));

The downfall is that it is opt-in (you have to explicitly use transientOptions), at least on the root component.

Hi,

just I want to say that I have used this solution and works great. Would be an option to add a helper function exportable from Emotion and thats all? It can be maybe customizable with a regex pattern maybe?

@callmeberzerker
Copy link

has there been any progress on this or any changes of opinion if it should be done? this is really needed for DX, plus styled-components API parity...

@Andarist
Copy link
Member

Andarist commented Oct 5, 2021

The API parity with Styled Components is not a goal. The mentioned concerns expressed here are still very much true. It also doesn't seem like there are a lot of people looking for this functionality since this thread isn't that busy at the end (I would probably have expected it to be much more busy based on this being in SC and all). This also has an impact on our consumers and I would be wary about including support for this in the current major version.

@AlexSapoznikov
Copy link

I also just started using emotion instead of styled-components and this is the first inconvenience that I encountered. I really hope that emotion will start using $ for transient props like styled-components in order to make devs life easier. It's just uncomfortable and ugly to pass shouldForwardProp to styled all the time.

@maapteh
Copy link

maapteh commented Oct 26, 2021

@Andarist i guess people don't reply on old topics, and some people don't even know its existence in SC.

At first i was also not bothered, thought about the nasty wrapper and thought i dont want to leave my code like this. So when i used styled components it was easy to understand what props should not be added to the component. Now i did use the same pattern and all of a sudden i have to change prop hasImage:boolean into hasimage:string since in the end it expects them to be DOM attributes which they are not.

I can understand this can be breaking by people using a $props convention (who on hell, but yes Maybe) for wanting them up to the dom. So maybe it should be in the next major? But please make our lives a little more easier :)

@Andarist
Copy link
Member

Note that we conditionally forward props to the underlying DOM elements (this can be customized with shouldForwardProp). If your hasImage gets forwarded then it's probably because you are wrapping a custom component with styled and you don't set its shouldForwardProp correctly (or just because you are spreading all received props onto the host element).

@JanStevens
Copy link

A lot of people new to frontend code get bitten by this, rest spreading a bunch of props on a styled component, only to notice way to late that suddenly there are a bunch of additional properties on the HTML tag making it invalid, render ugly or other horrible things.

Transient props is an elegant solution, yea sure you can already do it now but its not convenient, passing around the same function for every styled component. Indeed since v5 release of material UI I expect a lot more people will get bitten by this eventually

@callmeberzerker
Copy link

A lot of people new to frontend code get bitten by this, rest spreading a bunch of props on a styled component, only to notice way to late that suddenly there are a bunch of additional properties on the HTML tag making it invalid, render ugly or other horrible things.

Transient props is an elegant solution, yea sure you can already do it now but its not convenient, passing around the same function for every styled component. Indeed since v5 release of material UI I expect a lot more people will get bitten by this eventually

Exactly, and I don't understand the resistance to implement this functionality. This is the only real reason why I am still using styled-components (or mui-styled-engine-sc) vs the default engine in material-ui@5.x which is emotion.

I don't wanna be bothered to create:

const TabsStyled = styled(MuiTabs, {
  shouldForwardProp: prop =>
    isPropValid(prop) && prop.startsWith('$')
})`
   color: ${props =>
    props.$myCustomVariantThatStartsWithDollarSign ? 'hotpink' : 'turquoise'};

`

And the last thing I need is yet another styled import (wrapper) in my codebase (babel-macros also don't work if the files are imported from elsewhere, but don't quote me on that). I currently have this issue on my watchlist but I would want clear indication from emotion team if they would add this, or we should just settle on using styled-components if we want this kind of functionality out of the box.

@sauldeleon
Copy link

sauldeleon commented Nov 2, 2021

Hi @callmeberzerker I agree with you that this is a very shitty way, but it can be improved like

import { transientOptions } from '@utils'

// just an example of a $property, thought it does not make so much sense this way lol
interface StyledProps {
  $darkTheme: boolean
}

export const MyComponent= styled('div', transientOptions)<StyledProps>`
  background-color: var(--surface-${props => props.$darkTheme});

having in some common file

import { CreateStyled } from '@emotion/styled'

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

Yes, it would be better with a default built in approach for emotion, but for the moment I use this way, and the code does not look too messy!

Original solution in this thread by @jmca

@maapteh
Copy link

maapteh commented Nov 3, 2021

Thank you @jmca in my case im using a third party UI library Chakra-UI and i now can prevent props from passing and throwing errors to me (prop needs to be lowerCase, value must be String etc). Codebase became much better now :)

Screenshot 2021-11-03 at 09 33 25

I still hope it will be made available in the core of Emotion.

@WIH9
Copy link

WIH9 commented Nov 4, 2021

I've recently migrated a project I'm working on to MUI v5 and started using emotion for the first time. Came to this thread after noticing an error about receiving a boolean value for a non-boolean attribute and was hoping for a neat solution to deal with it. Came across this for styled-components, and was hoping for similar functionality for emotion. I echo the sentiments of others who don't want to have to explicitly filter out props that we don't want to pass to the DOM.

@fwextensions
Copy link

I came to this thread from the same SC issue as @WilliamHoangUK, having needed transient props for the first time after moving to MUI v5. Having some sort of built-in solution would be preferable to the various workarounds.

For now, I'm just passing 1 or 0 for a boolean filled prop, which suppresses the warning, but does cause a filled="1" attribute to show up on the DOM element. Unsightly, but the easiest solution for this one case.

@Ethorsen
Copy link

Ethorsen commented Jan 13, 2022

We were using css-in-js, we were passing props to styles that were only meant to affect styles.

When we try to move to @emotion, this is one of the first real annoyance. I was already thinking about creating a filter to not forward any props prefixed with some tag. Were hoping it could applied globally.

Did not know about SC transiantProps but obviously I think its a great feature that is badly missing from emotion.

For now we'll create the re-usable transientOptions as explained further up, and apply it on-the-go for each component that need "style-only related props"

Hopefully @emotion will come up with an out-of-the-box way of doing this.

@xenostar
Copy link

xenostar commented Feb 22, 2022

I just switched a project from styled-components to emotion and have run into this issue within minutes after switching. Transient props feels like a must, and this feels like a major inconvenience. Creating wrapper exports or injecting extra functions/objects into every component that needs this is quite frankly absurd for a solution. Emotion should be a pleasure to use right out of the box, I shouldn't have to jump through all these hoops to do something as simple as pass props around.

@pkaufi
Copy link

pkaufi commented Mar 3, 2022

I agree that having this implemented would make my life much easier. We're also just in process of migrating from SC to emotion, and this is a huge pain point. If you don't want to support it out of the box, maybe a configuration could be added, where it's possible to add a default behaviour for shouldForwardProp?

@jongbelegen
Copy link

jongbelegen commented Mar 7, 2022

I agree that having this implemented would make my life much easier. We're also just in process of migrating from SC to emotion, and this is a huge pain point. If you don't want to support it out of the box, maybe a configuration could be added, where it's possible to add a default behaviour for shouldForwardProp?

I also would prefer overwriting the default behavior for shouldForwardProp, maybe through provider context or instance creation. We could document a clean way of having "styled-components" like transient props in emotion with this approach.

My case is totally different, I don't want SVG props to be passthrough in my normal components. I have styled-components with options like color, spacing etc. and I prefer just disabling all svg passthroughs.

@mbcod3
Copy link

mbcod3 commented Mar 20, 2022

Why does emotion forward boolean prop to dom element by default? If it doesnt big use case for transient props would be resolved.

@sshmaxime
Copy link

I use Material UI (MUI) and it took me quite some time to realize that it was coming from emotion (being the default UI engine of MUI). I had to change the UI engine of MUI back to Styled-Component. This is how to do it: https://mui.com/material-ui/guides/styled-engine/#how-to-switch-to-styled-components

@fxdave
Copy link

fxdave commented May 24, 2022

When I create an interface for an element:

interface DivProps {
   big: boolean
}

It means I don't want its props for the HTML element

const StyledDiv = styled('div', {
   shouldForwardProp: (prop) => prop !== 'big'
})<DivProps>({
   // ...
})

So, ideally, I would have to write this:

const StyledDiv = styled.div<DivProps>({
   // ...
})

To be honest, this library is messing two different sets of props together, and we are trying to split them up manually.
It's like the inheritance coding habit that messes two schemas together, which I and I hope others as well don't like. We can solve that by using composition that gives names to the different schemas. And the solution is simple here as well:

The definition stays this untouched:

interface DivProps {
   big: boolean
}
const StyledDiv = styled.div<DivProps>({
   // ...
})

However the usage will be different. There are 3 solutions:
The cleanest solution:

<StyledDiv elementProps={{ className: "something" }} styledProps={{ big: true }} /> 

With a dedicated elementProps:

<StyledDiv big={true} elementProps={{ className: "something" }} /> 

Or with a dedicated styledProps which would be strange for a WrapperComponent like StyledDiv:

<StyledDiv className="something" styledProps={{ big: true }} /> 

Side note: In languages like Rust there would be no other solution. You would have to go with the first one, unless you want to copy the props from the elementProps and the styledProps to a new struct, which would be a maintenance hell. Unfortunately JS allows these evil solutions like which we have now.

@KevinKra
Copy link

KevinKra commented Jul 7, 2022

TLDR emotion will probably never add this feature 👎🏻

@Monstarrrr
Copy link

As a more junior developer this is holding me back quite a lot for something as basic and important as passing props around.

@Nantris
Copy link
Contributor

Nantris commented Aug 31, 2022

The mentioned concerns expressed here are still very much true. It also doesn't seem like there are a lot of people looking for this functionality since this thread isn't that busy at the end

@Andarist I feel like this should be revisited, even if there's not a clear path forward. It's by far the most thumbs-upped issue on the tracker at this point with 51 currently - more than double the next highest.

@Ethorsen
Copy link

Ethorsen commented Sep 1, 2022

We all hope that some sort of mechanism will be implemented to help alleviate this problem. But after a few years, it just feels like the dev team wont do anything about it

skatingincentralpark added a commit to skatingincentralpark/Videohead that referenced this issue Oct 2, 2022
- Fix DOM element prop warning: Use transientOptions() from this link
emotion-js/emotion#2193 (comment)
Example of warning below
- Fix GIFs not having alt warning: add a default if caption doesn't
exist
- Compute custom aspect ratio if image is cropped in Sanity
- Add priority to first 7 rows of images
- Refactor props

Warning: React does not recognize the `backgroundColor` prop on a DOM
element. If you intentionally want it to appear in the DOM as a custom
attribute, spell it as lowercase `backgroundcolor` instead. If you
accidentally passed it from a parent component, remove it from the DOM
element.
@AlexSapoznikov
Copy link

Was getting another check on this thread after Oct 7, 2021. It's sad that this is still a problem, so avoiding emotion-js this time.

@Nantris
Copy link
Contributor

Nantris commented Nov 16, 2022

8 more thumbs-ups since my last comment about 75 days ago.

@ndeviant
Copy link

ndeviant commented Dec 8, 2022

Any updates on this?

@mlnarik3pg
Copy link

We would like to see this feature!

@MadPeachTM
Copy link

so, it has been 2 years since the start of this conversation... has emotion implemented this feature? Docs on their website don't work and i can't find an answer anywhere

@Ethorsen
Copy link

Ethorsen commented Apr 13, 2023

@MadPeachTM Most probably not

answer is right here, simple JS version (there are TS examples in the thread):

export const withTransientProps = {
  shouldForwardProp: (propName) => propName !== 'theme' && !propName.startsWith("$"),
};

Then add it when necessary

const MyStyledStuff = styled(Stuff, withTransiantProps)(({ $someTransiantProp }) => ({
}));

@ghost
Copy link

ghost commented Jul 11, 2023

Any plans to add this?

@akullpp
Copy link

akullpp commented Aug 10, 2023

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
Copy link

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
Copy link

elektronik2k5 commented Mar 22, 2024

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
Copy link

Touseef-ahmad commented May 30, 2024

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
Copy link

oigewan commented Jun 1, 2024

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.

@marano
Copy link

marano commented Oct 16, 2024

The lack of this feature is forcing us to migrate our app to using styled components instead. I don't really understand why this is so controversial. It is really weird setting up a new react app with emotion and then getting a bunch of react warnings about these invalid props being passed to the DOM and no simple way to omit them.

@Nantris
Copy link
Contributor

Nantris commented Oct 16, 2024

I don't get the sense Emotion is maintained anymore. I think we're all headed for styled-components in due time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests