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

add reserved "sc" prop, handling #1682

Open
wants to merge 4 commits into
base: develop
from

Conversation

Projects
None yet
7 participants
@probablyup
Contributor

probablyup commented Apr 12, 2018

since it doesn't appear that JSX namespace attributes will land
in the near future, moving to use of a reserved, memorable prop
allows us to drop the attribute whitelist in a future major version

const Thing = styled.div`
  color: ${p => p.sc.color};
`;

<Thing sc={{ color: 'red' }} />

@probablyup probablyup requested review from kitten and mxstbr Apr 12, 2018

@probablyup probablyup changed the title from add reserved sc attribute handling to add reserved sc attribute, handling Apr 12, 2018

Show outdated Hide outdated src/models/StyledComponent.js
Show outdated Hide outdated src/models/StyledComponent.js
Show outdated Hide outdated src/models/StyledComponent.js
@mxstbr

How much of a bundle-size change will this bring to the table?

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Apr 12, 2018

Contributor

@mxstbr near-zero since we already have to consume PropTypes for the old context system

Contributor

probablyup commented Apr 12, 2018

@mxstbr near-zero since we already have to consume PropTypes for the old context system

probablyup added some commits Apr 12, 2018

add reserved sc attribute handling
since it doesn't appear that JSX namespace attributes will land
in the near future, moving to use of a reserved, memorable prop
allows us to drop the attribute whitelist in a future major version
@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Apr 16, 2018

Contributor

@kitten removed all the controversial bits

Contributor

probablyup commented Apr 16, 2018

@kitten removed all the controversial bits

@probablyup probablyup changed the title from add reserved sc attribute, handling to add reserved "sc" prop, handling Apr 16, 2018

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 18, 2018

Member

I don't think sc is a good name for this.

Component props are its public API. If a component uses SC, it used to be its implementation detail, but now it’s exposed. This makes it harder to swap SC with other implementations, and leaves people who migrate away from SC with technical debt in the form of a weird property that they might have to keep in their API for backward compat or because they don’t have time to refactor. It also makes the API of any SC slightly more confusing.

I understand the temptation to save typing but I’m not sure it’s the right instinct here. If you go this road I would suggest picking a name that has some meaning outside of SC. For example:

const Thing = styled.div`
  color: ${p => p.variant.color};
`;

<Thing variant={{ color: 'red' }} />
Member

gaearon commented Apr 18, 2018

I don't think sc is a good name for this.

Component props are its public API. If a component uses SC, it used to be its implementation detail, but now it’s exposed. This makes it harder to swap SC with other implementations, and leaves people who migrate away from SC with technical debt in the form of a weird property that they might have to keep in their API for backward compat or because they don’t have time to refactor. It also makes the API of any SC slightly more confusing.

I understand the temptation to save typing but I’m not sure it’s the right instinct here. If you go this road I would suggest picking a name that has some meaning outside of SC. For example:

const Thing = styled.div`
  color: ${p => p.variant.color};
`;

<Thing variant={{ color: 'red' }} />
@mxstbr

This comment has been minimized.

Show comment
Hide comment
@mxstbr

mxstbr Apr 18, 2018

Member

That sounds sensible to me @gaearon

Member

mxstbr commented Apr 18, 2018

That sounds sensible to me @gaearon

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Apr 18, 2018

Contributor

@gaearon how would you address the technical requirement of trying to avoid a prop that may already be in their codebase?

This change effectively punches out that prop such that the underlying component or JSX would not receive it. That’s why I specifically picked “sc”, since it’s unlikely it would be used for other purposes and is memorable.

In terms of switching libraries, that’s a moot point unless other libs implement similar functionality, and even then is very easy to codemod.

Contributor

probablyup commented Apr 18, 2018

@gaearon how would you address the technical requirement of trying to avoid a prop that may already be in their codebase?

This change effectively punches out that prop such that the underlying component or JSX would not receive it. That’s why I specifically picked “sc”, since it’s unlikely it would be used for other purposes and is memorable.

In terms of switching libraries, that’s a moot point unless other libs implement similar functionality, and even then is very easy to codemod.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 18, 2018

Member

I think it’s more important to create a good API for all future users than to lock them into an intentionally obscure name because of past users. You’re going to do a breaking change anyway.

Member

gaearon commented Apr 18, 2018

I think it’s more important to create a good API for all future users than to lock them into an intentionally obscure name because of past users. You’re going to do a breaking change anyway.

@kitten

This comment has been minimized.

Show comment
Hide comment
@kitten

kitten Apr 18, 2018

Member

Definitely agreed here; I'd love to finally put an end to the issue that is being addressed here, and I can accept that not everyone will be happy (heck, I think it's fine as it is), but sc can be improved upon.

That said, variant seems pretty good to me; The only problem would be that it's not quite suitable for every problem that one can throw at this API, but it's certainly suitable for the ones we'd like to be thrown at it 👍

Member

kitten commented Apr 18, 2018

Definitely agreed here; I'd love to finally put an end to the issue that is being addressed here, and I can accept that not everyone will be happy (heck, I think it's fine as it is), but sc can be improved upon.

That said, variant seems pretty good to me; The only problem would be that it's not quite suitable for every problem that one can throw at this API, but it's certainly suitable for the ones we'd like to be thrown at it 👍

@probablyup probablyup added the 4.0 label Apr 19, 2018

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Apr 19, 2018

Contributor

Going to think on the naming more... I'm sure there's a middle ground that's a little more unique.

@mxstbr dropping the validAttr code after this change reduces the bundle size by 2.5kB gzipped 😲

Contributor

probablyup commented Apr 19, 2018

Going to think on the naming more... I'm sure there's a middle ground that's a little more unique.

@mxstbr dropping the validAttr code after this change reduces the bundle size by 2.5kB gzipped 😲

@diondiondion

This comment has been minimized.

Show comment
Hide comment
@diondiondion

diondiondion Apr 19, 2018

I'd love to finally put an end to the issue that is being addressed here

I still consider this to be more of a lazy workaround than a real solution, and it kills a big part of styled-component's appeal to me. :(

I assume you're not considering anymore to add a method for specifying (near the component definition) which props not to pass through?

And if not, do you really expect any shared component library to make your reserved prop part of their public API?

Or is your position that if we want to hide the reserved prop, we should "just" create an intermediate component that maps top-level props to nested props?

diondiondion commented Apr 19, 2018

I'd love to finally put an end to the issue that is being addressed here

I still consider this to be more of a lazy workaround than a real solution, and it kills a big part of styled-component's appeal to me. :(

I assume you're not considering anymore to add a method for specifying (near the component definition) which props not to pass through?

And if not, do you really expect any shared component library to make your reserved prop part of their public API?

Or is your position that if we want to hide the reserved prop, we should "just" create an intermediate component that maps top-level props to nested props?

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Apr 19, 2018

Contributor

@diondiondion why would this kill some appeal? You can still use regular props and reference them in your styles... the only difference would be “variant” or whatever isn’t passed through to the underlying component/DOM. Purely a way to opt out of garbage in the DOM.

Contributor

probablyup commented Apr 19, 2018

@diondiondion why would this kill some appeal? You can still use regular props and reference them in your styles... the only difference would be “variant” or whatever isn’t passed through to the underlying component/DOM. Purely a way to opt out of garbage in the DOM.

@diondiondion

This comment has been minimized.

Show comment
Hide comment
@diondiondion

diondiondion Apr 19, 2018

@probablyup Isn't the idea to get rid of the prop whitelist? So, correct me if I'm wrong, but it seems like the choice will be between using the variant prop and having garbage in the DOM, which of course isn't acceptable normally, so there's not much of a choice in reality.

That's why I was asking:

I assume you're not considering anymore to add a method for specifying (near the component definition) which props not to pass through?

diondiondion commented Apr 19, 2018

@probablyup Isn't the idea to get rid of the prop whitelist? So, correct me if I'm wrong, but it seems like the choice will be between using the variant prop and having garbage in the DOM, which of course isn't acceptable normally, so there's not much of a choice in reality.

That's why I was asking:

I assume you're not considering anymore to add a method for specifying (near the component definition) which props not to pass through?

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Apr 19, 2018

Contributor

Yes, to get rid of the whitelist and make our prop behavior a little less magical / maintenance-heavy. As it is, the current strategy is kind of cruddy because it misses all-lowercase prop names used for styling.

Adding a prop to specify which props to not pass along is interesting. I could see that evolving into a community pattern perhaps. The nice thing is that can be added as an enhancement down the line, since it would involve a lot of manual setup.

Contributor

probablyup commented Apr 19, 2018

Yes, to get rid of the whitelist and make our prop behavior a little less magical / maintenance-heavy. As it is, the current strategy is kind of cruddy because it misses all-lowercase prop names used for styling.

Adding a prop to specify which props to not pass along is interesting. I could see that evolving into a community pattern perhaps. The nice thing is that can be added as an enhancement down the line, since it would involve a lot of manual setup.

@kitten

This comment has been minimized.

Show comment
Hide comment
@kitten

kitten Apr 19, 2018

Member

@probablyup What if we'd expose css-x props as p => p.css.x? Just throwing ideas into the mix

Edit: It should also be quite safe to assume that barely anyone is using props like css-name / css-x and such

Member

kitten commented Apr 19, 2018

@probablyup What if we'd expose css-x props as p => p.css.x? Just throwing ideas into the mix

Edit: It should also be quite safe to assume that barely anyone is using props like css-name / css-x and such

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 19, 2018

Member

I want to also express concern about any "remapping" scheme based on names. It's not very efficient to do lookups and object manipulations like this based on dynamic keys. It's also going to make it harder for future React compilation projects to deal with styled components.

Member

gaearon commented Apr 19, 2018

I want to also express concern about any "remapping" scheme based on names. It's not very efficient to do lookups and object manipulations like this based on dynamic keys. It's also going to make it harder for future React compilation projects to deal with styled components.

@kitten

This comment has been minimized.

Show comment
Hide comment
@kitten

kitten Apr 19, 2018

Member

@gaearon that's a good point; I'm basically just proposing JSX namespaces without it being a thing 😉

Member

kitten commented Apr 19, 2018

@gaearon that's a good point; I'm basically just proposing JSX namespaces without it being a thing 😉

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 19, 2018

Member

Yeah, I understand the intent, but the more “static” the final scheme is, the better. Anything that involves dynamic transformations, heuristics based on names or prop order, will cause problems later down the road.

Member

gaearon commented Apr 19, 2018

Yeah, I understand the intent, but the more “static” the final scheme is, the better. Anything that involves dynamic transformations, heuristics based on names or prop order, will cause problems later down the road.

@diondiondion

This comment has been minimized.

Show comment
Hide comment
@diondiondion

diondiondion Apr 19, 2018

Adding a prop to specify which props to not pass along is interesting.

Glamourous has that option, it's called filteredProps. You just pass it an array of prop names to filter (see glamourous PR).

glamorous(ChildComponent, {
  filterProps: ['foo', 'bar'],
})

In my mind this would be the perfect solution for styled-components as well. It requires a little bit more typing when creating the component, but lets us define a component's API freely, with no downsides. Plus it'll work equally well with html tags and wrapped components and is easy to teach.

When I suggested this last year, @gaearon has pointed out that he'd prefer an approach where the filtered props aren't passed as strings in an array, but rather a function like filterProps(({props, to, filter, ...rest}) => rest), but that comes with its own downsides (even more typing, creates unused vars which linters don't like, it's less focused on this specific issue & can be (ab)used for other stuff).

So, yeah.. sorry if I'm derailing the PR discussion a bit, I just care about this issue a lot and thought it'd be worth reiterating my concerns. If you want to read more about this, check out issue #439, but it is a bit of a long read. ;)

diondiondion commented Apr 19, 2018

Adding a prop to specify which props to not pass along is interesting.

Glamourous has that option, it's called filteredProps. You just pass it an array of prop names to filter (see glamourous PR).

glamorous(ChildComponent, {
  filterProps: ['foo', 'bar'],
})

In my mind this would be the perfect solution for styled-components as well. It requires a little bit more typing when creating the component, but lets us define a component's API freely, with no downsides. Plus it'll work equally well with html tags and wrapped components and is easy to teach.

When I suggested this last year, @gaearon has pointed out that he'd prefer an approach where the filtered props aren't passed as strings in an array, but rather a function like filterProps(({props, to, filter, ...rest}) => rest), but that comes with its own downsides (even more typing, creates unused vars which linters don't like, it's less focused on this specific issue & can be (ab)used for other stuff).

So, yeah.. sorry if I'm derailing the PR discussion a bit, I just care about this issue a lot and thought it'd be worth reiterating my concerns. If you want to read more about this, check out issue #439, but it is a bit of a long read. ;)

@kitten

This comment has been minimized.

Show comment
Hide comment
@kitten

kitten Apr 19, 2018

Member

@diondiondion the issue in #439 is frankly hopeless as it's gone through every single option with every single opinion. That's not saying that this PR is useless or what we'll definitely go for, but at this point it's a tough call either way.

@probablyup Let's park this for a bit please 😉 I think it's been clear that we need another angle to this — which I don't believe will be easy — or some time to decide whether we'll go for this temporarily.

Either way, I'm not sure whether we can get rid of validAttrs quite so soon even with a new approach

Member

kitten commented Apr 19, 2018

@diondiondion the issue in #439 is frankly hopeless as it's gone through every single option with every single opinion. That's not saying that this PR is useless or what we'll definitely go for, but at this point it's a tough call either way.

@probablyup Let's park this for a bit please 😉 I think it's been clear that we need another angle to this — which I don't believe will be easy — or some time to decide whether we'll go for this temporarily.

Either way, I'm not sure whether we can get rid of validAttrs quite so soon even with a new approach

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Apr 20, 2018

Contributor

Yeah I wasn’t planning on landing this until 4.0 anyway. Sounds like more discussion is needed.

Contributor

probablyup commented Apr 20, 2018

Yeah I wasn’t planning on landing this until 4.0 anyway. Sounds like more discussion is needed.

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Jun 22, 2018

Contributor

Another possibility that came up during a convo:

Make the special prop "theme" and then shallow merge it against the outer theme if it exists.

Example:

const StyledButton = styled.button`
  background-color: ${p => p.theme.backgroundColor};
  color: ${p => p.theme.color};
`;

// usage
// <StyledButton theme={{ color: 'red' }} />

// with a ThemeProvider, the local theme prop is shallow applied...

<ThemeProvider theme={{ color: 'green', backgroundColor: 'blue' }}>
  <StyledButton theme={{ color: 'red' }} />
</ThemeProvider>

// the button would be red
// and the background would remain blue

Pros

  • Only one place to look for theming-related props: props.theme
  • Very easy to do a local override of props.theme without having to use an interim <ThemeProvider> wrapper
  • Less chance of prop collision with other libraries & setups since this is an established pattern
  • Clear delineation of what is a "visual" prop vs what are custom attributes and/or real attributes

Considerations

  • props.theme must always be an object (I haven't seen people use non-object themes in the wild, but it's possible)
  • automatic shallow merge might be confusing
  • The global theme passed down via would pollute the "prop namespace" for nested components. Basically, if your global theme contains any properties of the same name as any one of your styled-component's props used for styling, there'll be overlap & conflicts & potentially accidentally applied styles.
  • no shorthand for boolean theme props <Box fullWidth />
Contributor

probablyup commented Jun 22, 2018

Another possibility that came up during a convo:

Make the special prop "theme" and then shallow merge it against the outer theme if it exists.

Example:

const StyledButton = styled.button`
  background-color: ${p => p.theme.backgroundColor};
  color: ${p => p.theme.color};
`;

// usage
// <StyledButton theme={{ color: 'red' }} />

// with a ThemeProvider, the local theme prop is shallow applied...

<ThemeProvider theme={{ color: 'green', backgroundColor: 'blue' }}>
  <StyledButton theme={{ color: 'red' }} />
</ThemeProvider>

// the button would be red
// and the background would remain blue

Pros

  • Only one place to look for theming-related props: props.theme
  • Very easy to do a local override of props.theme without having to use an interim <ThemeProvider> wrapper
  • Less chance of prop collision with other libraries & setups since this is an established pattern
  • Clear delineation of what is a "visual" prop vs what are custom attributes and/or real attributes

Considerations

  • props.theme must always be an object (I haven't seen people use non-object themes in the wild, but it's possible)
  • automatic shallow merge might be confusing
  • The global theme passed down via would pollute the "prop namespace" for nested components. Basically, if your global theme contains any properties of the same name as any one of your styled-component's props used for styling, there'll be overlap & conflicts & potentially accidentally applied styles.
  • no shorthand for boolean theme props <Box fullWidth />

@probablyup probablyup changed the base branch from master to develop Aug 11, 2018

@probablyup probablyup added 5.0 and removed 4.0 labels Aug 12, 2018

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Aug 20, 2018

Member

Adding a con to the list above - no shorthand for boolean theme props (as in <Box fullWidth />)

Member

Andarist commented Aug 20, 2018

Adding a con to the list above - no shorthand for boolean theme props (as in <Box fullWidth />)

@probablyup probablyup referenced this pull request Aug 21, 2018

Open

[Tentative] 5.0 Roadmap #1934

0 of 5 tasks complete
@diondiondion

This comment has been minimized.

Show comment
Hide comment
@diondiondion

diondiondion Sep 11, 2018

Make the special prop "theme" and then shallow merge it against the outer theme if it exists.

Another con for using the theme prop: The global theme passed down via <ThemeProvider> would massively pollute the "prop namespace" for nested components. Basically, if your global theme contains any properties of the same name as any one of your styled-component's props used for styling, there'll be overlap & conflicts & potentially accidentally applied styles.

A global theme should be something you only access intentionally, and this solution blurs the line between component props & global theme props.

diondiondion commented Sep 11, 2018

Make the special prop "theme" and then shallow merge it against the outer theme if it exists.

Another con for using the theme prop: The global theme passed down via <ThemeProvider> would massively pollute the "prop namespace" for nested components. Basically, if your global theme contains any properties of the same name as any one of your styled-component's props used for styling, there'll be overlap & conflicts & potentially accidentally applied styles.

A global theme should be something you only access intentionally, and this solution blurs the line between component props & global theme props.

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Sep 11, 2018

Contributor

@diondiondion added your point to the comment. That's true. We could do something like reassign the theme from context to theme.context.

Then you'd use it like this:

const Component = styled.div`
  color: ${p => p.theme.context.color};
  background: ${p => p.theme.background};
`

<ThemeProvider theme={{ color: 'red' }}>
    <Component background="blue" />
</ThemeProvider>
Contributor

probablyup commented Sep 11, 2018

@diondiondion added your point to the comment. That's true. We could do something like reassign the theme from context to theme.context.

Then you'd use it like this:

const Component = styled.div`
  color: ${p => p.theme.context.color};
  background: ${p => p.theme.background};
`

<ThemeProvider theme={{ color: 'red' }}>
    <Component background="blue" />
</ThemeProvider>
@srilq

This comment has been minimized.

Show comment
Hide comment
@srilq

srilq Sep 11, 2018

Member

Why does the component theme have to pollute the main theme? I would see it working like this, where the outer Component instance has a blue background, and the inner Component goes back to being red again as the theme variation isn't passed down.

const Component = styled.div`
  color: ${p => p.theme.color};
  background: ${p => p.theme.background};
`

<ThemeProvider theme={{ color: 'red', background: 'red' }}>
  <Component theme={{ background: 'blue' }}>
    <Component />
  </Component>
</ThemeProvider>

If the component theme does pollute the main theme, then why not just use a nested ThemeProvider? I'm not sure what the value of the prop is at that point, but maybe I'm missing context.

Member

srilq commented Sep 11, 2018

Why does the component theme have to pollute the main theme? I would see it working like this, where the outer Component instance has a blue background, and the inner Component goes back to being red again as the theme variation isn't passed down.

const Component = styled.div`
  color: ${p => p.theme.color};
  background: ${p => p.theme.background};
`

<ThemeProvider theme={{ color: 'red', background: 'red' }}>
  <Component theme={{ background: 'blue' }}>
    <Component />
  </Component>
</ThemeProvider>

If the component theme does pollute the main theme, then why not just use a nested ThemeProvider? I'm not sure what the value of the prop is at that point, but maybe I'm missing context.

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Sep 11, 2018

Contributor

@srilq yeah your example is the original proposal for how it should work. I agree with you.

Contributor

probablyup commented Sep 11, 2018

@srilq yeah your example is the original proposal for how it should work. I agree with you.

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