Fix don't omit theme at runtime #4129
Conversation
| @@ -143,7 +143,7 @@ function useStyledComponentImpl<Props extends object>( | |||
| if (context[key] === undefined) { | |||
| // Omit undefined values from props passed to wrapped element. | |||
| // This enables using .attrs() to remove props, for example. | |||
| } else if (key[0] === '$' || key === 'as' || key === 'theme') { | |||
There was a problem hiding this comment.
I'd rather not do this, can use useTheme to get the theme in your component if needed.
There was a problem hiding this comment.
Sorry, I didn't know how...
Could you please show me an example??
There was a problem hiding this comment.
There was a problem hiding this comment.
@probablyup I think there is misunderstanding about the intention of this change.
It is not about to acquire the theme via the theme prop or via the useTheme hook. It is not about the styled-components theming mechanism at all.
It is about other libs relying on the "theme" prop which is now "killed" by the styled-components wrapper.
Many internal components of react-select passing "theme" props around. When you style them via styled-components, this prop is gone and we ran into a runtime error.
This was also mentioned in #4074.
So changing the line 146 to not remove the "theme" prop fixes that issue with react-select.
There was a problem hiding this comment.
@quantizor Could we please have this PR added or implement other change to fix the #4074. issue due to which it is impossible to use react-select with styled components?
There was a problem hiding this comment.
The only thing I'm not sure about is what should happen if a theme prop is passed that is incompatible with the styled-components theme. The styles still need the context theme to work, whereas the theme being described here has nothing to do with styled-components.
There was a problem hiding this comment.
Maybe the theme should be a second argument to style interpolations instead of being lumped in with all props. That would disambiguate the two, but we wouldn't be able to remove the s-c theme from the props object until v7 since it would be a breaking change.
There was a problem hiding this comment.
Further complicating this, we allow the s-c theme to be overridden via theme prop as a general feature...
There was a problem hiding this comment.
@quantizor Not sure that I understand in details what was changed in s-c v6 but it worked in v5. Here is the sandbox. If you downgrade s-c to 5.3.11 there is no any error.
|
I prepared an alternate implementation that will not leak the |
|
ThanQ |
#4074
Include the
themein the execution props.Some UI component libraries require a theme at runtime. (e.g. react-select)
If you skip here, you will not be able to refer to the theme when overwriting the theme, resulting in a runtime error.
In fact, if you try using the codesandbox in the issue comment, A runtime error occurs because the theme cannot be referenced.