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

Add Documentation for a Migration HOC #1475

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ module.exports = {
{
files: ['docs/*.md', 'docs/**/*.md'],
rules: {
'no-console': 'off'
'no-console': 'off',
'func-names': 'off'
}
},
{
Expand Down
134 changes: 134 additions & 0 deletions docs/react-jss-hoc-migration.md
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})
Copy link
Member

@kof kof Mar 23, 2021

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.

Copy link
Member

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

Copy link
Member

@kof kof Mar 23, 2021

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:

  • hooks
  • hoc
  • theming
  • css() (unofficial yet)
  • styled() (unofficial yet)
  • JssProvider (only needed for customization and different setups for subtrees or SSR won't be needed by many people or only in one place)

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

Copy link
Member Author

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. 😅


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.
2 changes: 1 addition & 1 deletion docs/react-jss.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ React-JSS integrates [JSS](https://github.com/cssinjs/jss) with React using the

Try it out in the [playground](https://codesandbox.io/s/j3l06yyqpw).

**The HOC based API is deprecated as of v10 and may be removed in a future version. You can still perform a lazy migration as described [here](https://reacttraining.com/blog/using-hooks-in-classes/). HOC specific docs are available [here](./react-jss-hoc.md).**
**The HOC-based API is deprecated as of v10 and may be removed in a future version. You can still perform a lazy migration as described [here](react-jss-hoc-migration.md). Documentation for the deprecated HOC-based API is available [here](react-jss-hoc.md).**

### Benefits compared to using the core JSS package directly:

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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 tsc, their version of TypeScript will be used.

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 check:ts script worked fine without having to modify any definitions.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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

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.

Copy link
Member

Choose a reason for hiding this comment

The 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"
}
Expand Down
16 changes: 9 additions & 7 deletions packages/react-jss/tests/types/withStyles.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ interface MyTheme {
color: 'red'
}

function SimpleComponent(props: MyProps) {
return <div>{props.property}</div>
class SimpleComponent extends React.Component<MyProps> {
render() {
return <div>{this.props.property}</div>
}
}

// Intended to test the output of withStyles to make sure the props are still valid
Expand Down Expand Up @@ -137,7 +139,7 @@ ComponentTest = () => <ResultingComponent property="" />
/* -------------------- Failing Cases -------------------- */

// A function argument cannot provide another defined theme type conflicting with `undefined`
function failingFunctionRedefineTheme(theme: MyTheme): Styles<string, unknown, any> {
function failingFunctionNullTheme(theme: MyTheme): Styles<string, unknown, null> {
return {
someClassName: '',
anotherClassName: {
Expand All @@ -146,7 +148,7 @@ function failingFunctionRedefineTheme(theme: MyTheme): Styles<string, unknown, a
}
}

function passingFunctionUnknownTheme(theme: MyTheme): Styles<string, unknown, unknown> {
function passingFunctionAnyTheme(theme: MyTheme): Styles<string, unknown, any> {
return {
someClassName: '',
anotherClassName: {
Expand All @@ -155,7 +157,7 @@ function passingFunctionUnknownTheme(theme: MyTheme): Styles<string, unknown, un
}
}

function passingFunctionNullTheme(theme: MyTheme): Styles<string, unknown, null> {
function passingFunctionUnknownTheme(theme: MyTheme): Styles<string, unknown, unknown> {
return {
someClassName: '',
anotherClassName: {
Expand All @@ -165,6 +167,6 @@ function passingFunctionNullTheme(theme: MyTheme): Styles<string, unknown, null>
}

// @ts-expect-error
withStyles(failingFunctionRedefineTheme)(SimpleComponent)
withStyles(failingFunctionNullTheme)(SimpleComponent)
withStyles(passingFunctionAnyTheme)(SimpleComponent)
withStyles(passingFunctionUnknownTheme)(SimpleComponent)
withStyles(passingFunctionNullTheme)(SimpleComponent)
Loading