-
-
Notifications
You must be signed in to change notification settings - Fork 399
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 Documentation for a Migration HOC #1475
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,134 @@ | ||
# Migrating the `withStyles` HOC | ||
|
||
Although we recommend using the new hooks, it's possible that you have class components that cannot be migrated easily at this time. In that case, you can create your own higher order component (HOC) from the provided hooks. This way, you'll have a HOC that stays up-to-date with the latest features, and you'll still have the option of fully migrating to hooks at your own convenience. | ||
|
||
A simple solution may look something like this: | ||
|
||
```jsx | ||
import React from 'react' | ||
import {createUseStyles, useTheme} from 'react-jss' | ||
|
||
/** | ||
* Creates a Higher Order Component that injects the CSS specified in `styles`. | ||
* @param styles | ||
*/ | ||
function withStyles(styles) { | ||
return function(WrappedComponent) { | ||
const useStyles = createUseStyles(styles) | ||
|
||
const StyledComponent = props => { | ||
const {classes, ...passThroughProps} = props | ||
const theme = useTheme() | ||
const reactJssClasses = useStyles({...passThroughProps, theme}) | ||
|
||
return <WrappedComponent {...passThroughProps} classes={reactJssClasses} /> | ||
} | ||
|
||
StyledComponent.displayName = `withStyles(${WrappedComponent.name})` | ||
|
||
return StyledComponent | ||
} | ||
} | ||
|
||
export default withStyles | ||
``` | ||
|
||
Note that `useTheme` can be excluded if your application is not using a theme. | ||
|
||
To learn more about HOCs, see [react's documentation](https://reactjs.org/docs/higher-order-components.html). Since our HOC uses the `createUseStyles` hook under the hood, you can use the regular [hooks documentation](react-jss.md) for help with defining your `styles` objects. | ||
|
||
**Warning**: Because this HOC makes use of hooks, it cannot be used as a decorator. | ||
|
||
## Adding TypeScript to Your HOC | ||
|
||
If you're using TypeScript, you'll likely want to add types for your custom `withStyles` HOC, like so: | ||
|
||
```tsx | ||
import React from 'react' | ||
import {createUseStyles, useTheme, Styles} from 'react-jss' | ||
|
||
type ReactJSSProps = {classes?: ReturnType<ReturnType<typeof createUseStyles>>} | ||
|
||
/** | ||
* Creates a Higher Order Component that injects the CSS specified in `styles`. | ||
* @param styles | ||
*/ | ||
function withStyles<C extends string, Pr extends ReactJSSProps, T>( | ||
styles: Styles<C, Pr, T> | ((theme: T) => Styles<C, Pr>) | ||
) { | ||
return function<P extends Pr, S>(WrappedComponent: React.ComponentClass<P, S>): React.FC<P> { | ||
const useStyles = createUseStyles<C, P, T>(styles) | ||
|
||
const StyledComponent: React.FC<P> = (props: P) => { | ||
const {classes, ...passThroughProps} = props | ||
const theme = useTheme<T>() | ||
const reactJssClasses = useStyles({...(passThroughProps as P), theme}) | ||
|
||
return <WrappedComponent {...passThroughProps as P} classes={reactJssClasses} /> | ||
} | ||
|
||
StyledComponent.displayName = `withStyles(${WrappedComponent.name})` | ||
|
||
return StyledComponent | ||
} | ||
} | ||
|
||
export default withStyles | ||
``` | ||
|
||
This typed HOC enforces consistency with your `RuleNames` and `Theme`. It also enforces consistency between the `Props` you give to `Styles` and the ones you give to your component. | ||
|
||
You'll notice that here, we've typed the HOC to accept only class components as arguments. This is because you should be using the provided hooks for your functional components; not only do hooks provide a simpler interface, but they also help clarify which props actually belong to your component. | ||
|
||
## Migrating from Decorators | ||
|
||
Because this custom HOC makes use of hooks (which are [unusable in class components](https://reactjs.org/docs/hooks-faq.html#:~:text=You%20can't%20use%20Hooks,implementation%20detail%20of%20that%20component.)), you won't be able to use this HOC as a decorator. If you are using decorators in your project, you'll likely have to migrate your code from this: | ||
|
||
```javascript | ||
import React from 'react' | ||
import decorator1 from 'some-hoc-library' | ||
import decorator2 from 'another-hoc-library' | ||
// ... | ||
import withStyles from 'path/to/custom-hoc' | ||
|
||
const styles = { | ||
/* ... */ | ||
} | ||
|
||
@decorator1 | ||
@decorator2 | ||
// ... | ||
@withStyles(styles) | ||
class MyComponent extends React.Component { | ||
// ... | ||
} | ||
|
||
export default MyComponent | ||
``` | ||
|
||
to this: | ||
|
||
```javascript | ||
import React from 'react' | ||
import decorator1 from 'some-hoc-library' | ||
import decorator2 from 'another-hoc-library' | ||
// ... | ||
import withStyles from 'path/to/custom-hoc' | ||
|
||
const styles = { | ||
/* ... */ | ||
} | ||
|
||
@decorator1 | ||
@decorator2 | ||
// ... | ||
class MyComponent extends React.Component { | ||
// ... | ||
} | ||
|
||
export default withStyles(styles)(MyComponent) | ||
``` | ||
|
||
If you find yourself using many decorators for your class components, consider migrating away from chained decorators to [composed function calls](https://reactjs.org/docs/higher-order-components.html#convention-maximizing-composability). This is a safer play in the long run since decorators still have not stabilized in the JS standard. | ||
|
||
If you don't use decorators or aren't familiar with them, then this won't be a concern for you. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,7 +99,7 @@ | |
"rollup-plugin-terser": "^7.0.2", | ||
"shelljs": "^0.8.2", | ||
"sinon": "4.5.0", | ||
"typescript": "^3.7.0", | ||
"typescript": "^4.2.3", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding documentation and increasing this version is probably not a good way to create a PR, because who knows what side effects this could have unrelated to the docs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I know. 😔 I was having a hard time thinking about this because the new tests that I wrote literally won't work without the new TypeScript version -- particularly the failing tests. I could remove the version update, but I'd also have to remove the new tests. I can separate them if desirable though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does one update typescipt dependency? Does it have a consequence for the user? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's truly a dev dependency and docs are new, maybe we just need to hint the version of typescript they depend on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, because TypeScript is a devDependency, updating our own TypeScript version shouldn't affect other users. When the end user checks their types through something like So the potential for breaking changes would only be a concern if we changed type definitions. However, we haven't changed any type definitions -- only tests. And the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be fine then, do people mark docs and examples with TS version required for an example to work? Maybe a good idea to add a label in such cases There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe we could give a small reminder to use the latest version of TypeScript? Thankfully, it's very unlikely that the user would feel any impact between v3 and v4. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Interestingly enough, I don't see people mention TypeScript versions in docs very often -- if ever. This is even the case for popular libraries. There seems to be a general assumption that people are using the latest version of TS. That might be because it's pretty uncommon to update TypeScript's major version and run into any errors -- especially any significant errors. Managing TS dependencies tends to be fairly easy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, lets wait for someone saying example doesn't work, until then we avoid adding versions :) |
||
"webpack": "^4.28.3", | ||
"zen-observable": "^0.6.0" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also merging of classes happening right now in withStyles, I am wondering if we should just rewrite the HOC to use hooks like this and make it a separate entry point so that it doesn't increase bundle size for everyone even if one doesn't have tree shaking
or alternatively just link them to the latest version current implementation that they can just copy it into their code base as is and not bother creating their own potentially incomplete versions.
Since we are not going to have to fix it any more, people will only need it to keep the current code working and use hooks directly for the new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially we would need to make it copy/pastable because currently withStyles reuses quite a bit code with hooks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought about it a bit more, I think we should start splitting react-jss into multiple packages:
By doing so we can keep them using the hocs interface from a separate package as it is now many more years, since it's not increasing bundle size
react-jss also has a generally big bundle size because of how bundlephobia doesn't know what is actually used, so it can not treeshake
by splitting into packages, users will know what exactly they are using and will see specific packages in bundlephobia
This is slightly off-top here, but if we go this route, we don't need to explain how to create own withStyles HOC, so it matters
I am also thinking that creating a new preset package with only plugins which most people expect to be inside, something comparable to emotion/SC from capabilities perspective, will largely improve bundle size and hence perception of the library
In addition to that a few things can be removed from JSS package that haven't been properly documented or used anyways and a few files can be entirely moved to a separate package so that jss core can also have a smaller bundle size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, I don't have time to respond to these comments, but I will respond later.
Just putting that out there since I responded to the other comment. Didn't want it to seem like I was ignoring the ones you gave here. 😅