[styles] Remove hoisting of static properties in HOCs#13698
Conversation
eps1lon
added
breaking change
|
Ironically, I have just remembered: styled-components/styled-components#1411 (comment) -> #9736. More food for thoughts. |
|
@oliviertassinari So we actually depend on hoisting. I guess we manually hoist this in Do we need the actual name though or would a truthy property behind a symbol suffice? |
|
We can hoist it manually. It's the only case I can think of that worth it. Why do you want to use a symbol? Some people set it manually too when writing custom components. |
|
Should we close this pull-request? Assuming everything is working now? |
|
I would still drop support for this since every argument I made is still valid. There is even an open PR on I would much prefer whitelisting internal statics for hoisting and leaving the rest to the users. |
|
Right, as you see fit :) |
c6a3175 to
a0fd220
Compare
a0fd220 to
0e1a67a
Compare
This reverts commit 0e1a67a. Could not figure out how to use next styles
5bbd38c to
7b48e00
Compare
|
It's a 50/50 🍀 chance we will have to revert this change. I hope we won't, but we are taking some risks, we will see 😎. |
7b48e00 to
ccd990a
Compare
eps1lon
changed the title
2 participants
BREAKING: Removes call of
hoist-non-react-staticsinwithStylesandwithTheme. This does only affect the@material-ui/stylespackage which is still in alpha.Migration for hoisting requirement:
import { withStyles } from '@material-ui/styles'; - export default withStyles(styles)(Component); + import hoistNonReactStatics from 'hoist-non-react-statics'; + export default hoistNonReactStatics(withStyles(styles)(Component), Component);This might be for some people a controversial change so I'm trying to explain the reasons to the best of my ability:
hoist-non-react-staticsis not maintained by the react teamThis is/was a major pain point when migrating to refs +
React.forwardReffromfindDOMNode. The react team added all these new features without considering the impact it had on a very popular pattern in the ecosystem. It caused false positives [Context Api] React 16.6.0 #13465 and hard to debug errors in our docs (hard to tell where the actual issue was but hoisting forwardRef.render into a class component was somehow bad).I'm fully aware that the react docs explicitly say that you should copy statics over but unless a better solution is found this part of the code needs extra maintenance for every react feature when a dependency should reduce the maintenance burden as much as possible. However the text only gives
relayas an example which leads me to believe that copying statics is more of an issue in facebooks code. Exporting those statics is a much better solution in my opinion.It is still doable in userland with e.g.
compose(hoistNonReactStatics(UnstyledComponent), withStyles(styles))(UnstyledComponent)from the recompose package. Since I suspect that the requirement for static hoisting is not a very common use case I rather give the responsibility to the consumer than to the library. Warning:recomposestill uses an outdated version ofhoist-non-react-statics. Use your own implementation with the latest version ofhoist-non-react-statics.goes against "a function should do one thing"
For me a good example where not following this is a code smell.
withStylesdid two things and one of them bad.Currently type information for non-react statics is lost anyway. So everybody who used flow or TypeScript didn't have proper statics support to begin with.
It's targeted at an alpha release
We can safely gauge the impact of this change and get better feedback. I'm also fully expecting that this is caused by ignorance on my part since I don't think I ever relied (consciously) on hoisting statics.
Reduced bundle size for everyone that does not need this feature (1KB i think)
Why not inline the dependency?
While this might improve the response time it doesn't solve the actual issue. This would also increase our workload even more beyond "just bumping a dependency version".