Add Documentation for a Migration HOC #1475
Conversation
* Added documentation for people to create a custom "lazy migration HOC" since the old HOC-based API is deprecated. - Includes information on JS, TS, and decorators. * Added tests to verify that the custom `withStyles` HOC behaves correctly. * Updated TypeScript to make its type checking more accurate. - Includes fixing/updating tests for withStyles.tsx * Loosed ESLint's rules for markdown files.
c4dcfe2
to
c72c1ad
Compare
|
Added one quick amend. I added a small simplification of the TS types. The less people have to understand/think about, the better. |
| const StyledComponent = props => { | ||
| const {classes, ...passThroughProps} = props | ||
| const theme = useTheme() | ||
| const reactJssClasses = useStyles({...passThroughProps, theme}) |
There was a problem hiding this comment.
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.
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.
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
There was a problem hiding this comment.
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. 😅
| @@ -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.
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.
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.
How does one update typescipt dependency? Does it have a consequence for the user?
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alright, lets wait for someone saying example doesn't work, until then we avoid adding versions :)
|
Dunno what to do with this pr @ITenthusiasm , I just merged #1508 which rewrites WithStyles to use hooks internally which helped a lot making sure the hoc is doing the same thing as it was before. Maybe now its time to continue with this plan and go to the next stage of removing WithStyles from the bundle. I will release it hopefully today and we can monitor if that has caused problems, after that we will know this 50LOC can be just safely copy pasted from an example. |
|
I've been a bit more preoccupied than I originally imagined as of recent. If
That sounds awesome. Quick question (forgive my ignorance), did |
|
We have one example with decorators, that has been there since many years and for 2 years we have the HOC syntax in that separate file discouraging using it We never fully deprecated the HOC though. When you say decorators, you mean I guess actual decorator syntax and I am not sure whats the state of the modern decorators, I think they don't work any more the way stage 2 proposal was, but if they do, they most likely compile to the same hoc one would write by hand |
Yes. I'm not fully certain what the state of modern decorators is either. 🤔 But basically, from what I recall, class-based HOCs in React are inherently structured in a way that makes them usable as decorators. I found this out at my old job a few months back. When testing some of these changes on that project, I discovered that HOCs that used hooks can't be used as decorators for class components. If decorators are already discouraged, it probably isn't a huge deal. Just something to be aware of. |
What Would You Like to Add/Fix?
This is a PR to add documentation for creating a custom higher order component (HOC) using the new hooks-based API. It includes explanations for both JS and TS users, and it provides some warnings for anyone previously using decorators. This PR satisfies #1459.
As with before, TypeScript tests are included for the documentation's TS example. (This proved useful/necessary, given that it's not quite the same as the deprecated HOC-based API.)
Todo