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

[Suggestion] Consider RenderProps instead of HOCs for WithStyles #8542

Closed
vyrotek opened this issue Oct 4, 2017 · 9 comments
Closed

[Suggestion] Consider RenderProps instead of HOCs for WithStyles #8542

vyrotek opened this issue Oct 4, 2017 · 9 comments

Comments

@vyrotek
Copy link

vyrotek commented Oct 4, 2017

Recently there has been some buzz around a talk calling out HOCs as the new Mixins and how they are full of problems. As I've been following the WithStyles TypeScript issues (e.g. #8447) I couldn't help feel like maybe the problem isn't a lack of TS features but rather a possibly a problem with the approach.

Here's an excellent write-up and video of what @mjackson calls "Render Props" and how they can do everything an HOC can and also solves several problems with them.

Use a Render Prop!

I highly recommend watching the video as he does an excellent job illustrating how the React community got to where it is now. (FWIW, MJ is also the author of ReactRouter which currently implements this pattern)

What might WithStyles look like using Render Props instead? Could it help provide better type safety by removing the need for the ! hack and provide more clarity as to what props WithStyles is providing?

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 4, 2017

Consider RenderProps

@vyrotek I thought this pattern was called compound components 🤔 .
This issue is interesting, thanks for opening it. However, I believe we have already been exploring this path:

I'm closing, feel free to argue with any of the points I have already be raising against it :).

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 4, 2017

Could it help provide better type safety by removing the need for the ! hack and provide more clarity as to what props WithStyles is providing?

You might be interested in this issue #8447. It's a tradeoff.

@vyrotek
Copy link
Author

vyrotek commented Oct 4, 2017

Good info. Thanks. I hadn't heard of the term "Compound Components" but I have heard it referred to as "Function as Children" as this comment did #7636 (comment)

Yeah, issue #8447 is what finally did it for me. I love TypeScript but I understand the concerns about not wanting to cater to a decorator feature that isn't finished yet. At the same time, I feel like the root problem is having withStyles mix in extra props which fights the whole concept of typing and we could run into prop conflicts.

So ultimately I guess your comment #7636 (comment) about "lifecycles > flexibility" ended the discussion. Although I don't fully understand the reasoning and probably disagree I suppose I'm satisfied knowing it had been discussed before.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 5, 2017

To summarize, I think that the issues with the pattern are the following by order of importance:

  1. It requires more boilerplate, It's slightly true for nominal use cases, but definitely painful when needed to access the classes in the lifecycles hooks.
  2. We shouldn't, as much as possible, sacrifices pattern the language is allowing for typing constraints. It should be the other way around.
  3. Performance, it's hard to tell without a benchmark, but given the fact we are creating a new function at each render x the number of components we have. It's definitely a small overhead.

Of course, I'm focusing on the cons, this has pros too!

@vyrotek
Copy link
Author

vyrotek commented Oct 7, 2017

  1. Hmm, I'll need to do more research on this.
  2. https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578
  3. Just because you can doesn't mean you should. Even without typing I don't like how much it still feels like mixins. Very large projects may bump into prop naming conflicts and source confusion.

Crazy thought. Could WithStyles support both approaches? I'm guessing 1. is the biggest concern?

Sorry for all the poking! I like what you guys are doing here. I'm evaluating several UI frameworks for a large project and the friction of using WithStyles with TypeScript (and justifying any issues to other devs) is my biggest concern.

@oliviertassinari
Copy link
Member

@vyrotek Thanks for the link, I will read it.

Crazy thought. Could WithStyles support both approaches?

Definitely, there is no technical limitation for this. It can even be implemented today on userland. Regarding supporting it on the library side. I'm not sure. It depends on the vision we have with the withStyles() Higher-order Component.

If you go through the docs, it says that it's our internal styling helpers that we expose for convenience to our users. It was first designed for the internal components needs. Users are free to use whatever styling solution they want on top of Material-UI. Of course, the direct access to the theme with withStyles() is great, but it can be replaced. For instance, with the withTheme() Higher-order component or styled-components theming.

Personally, I'm using withStyles() exclusively in userland for all the projects I'm working on. I have also the feeling that a large majority of our best testers do the same (based on the feedback I see).

I'm gonna ping @kof as the topic can be extended to react-jss.

@oliviertassinari
Copy link
Member

Sorry for all the poking! I like what you guys are doing here. I'm evaluating several UI frameworks for a large project and the friction of using WithStyles with TypeScript (and justifying any issues to other devs) is my biggest concern.

@vyrotek Let me know what you pick, all the feedbacks are welcomed!

@sebald Given the TypeScript definition of withStyles() is going to change significantly with the next beta release for the better, maybe it's time to add a TypeScript guide in the documentation? What do you think? I have added a very simple Flow guide recently.

@kof
Copy link
Contributor

kof commented Oct 7, 2017

I have an issue for a HOC-less interface since 2016) https://github.com/cssinjs/react-jss/issues/31 I reopen it now, because render functions are hype now)

Not sure though when it makes really sense. Because we got also dynamic values now.

@kof
Copy link
Contributor

kof commented Oct 7, 2017

What benefit exactly would it be for styling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants