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

Feedback from @JedWatson #222

Closed
kof opened this issue Apr 23, 2016 · 10 comments
Closed

Feedback from @JedWatson #222

kof opened this issue Apr 23, 2016 · 10 comments

Comments

@kof
Copy link
Member

kof commented Apr 23, 2016

This is a better place to discuss this very broad topic. @JedWatson posted a great feedback I would better discuss here.


Nice work, @kof. I've spent a lot of time thinking about the same thing, and done a lot of work recently with @jossmac and @josephg on proofs of concept around the same things.

After trying to use JSS in a production project that needs server-side rendering, we've found two major blockers; non-deterministic generation of class names is the big one (this is solved by aphrodite and css-modules) and having no way to rehydrate the JSS state on client-side render is the other (also solved by aphrodite, and related to their solution for deterministic class names)

Adding and removing stylesheets as they are used (which is done by react-jss) is nice, but also causes some problems related to the above. Maybe the solution here is a different package for react integration.

The other two major issues we've found, not specific to server-side rendering, and both solvable (imo) are:

  • Using wrapper components around every single React component styles with JSS is painful in a real-world application. It doubles the size of your component hierarchy, prevents use of statics, and makes using the react dev tools much harder. There has to be a better way to attach styles (aphrodite solves this by providing the css method, which is uglier than how you use jss, but on the whole is nicer than having wrapping components everywhere)
  • JSS styles have to be generated when the code is interpreted, not when the component is created or rendered. This is related to the react-jss implementation and my previous point. This prevents you from changing styles at run-time, e.g. allowing theme properties (such as border radius, for example) to be provided through props or context to a component. This is a huge limitation in terms of building a generic library of UI components.

To give some more background, that project I mentioned - we wrote it initially with JSS (and loved it, by the way, huge props for the work you have done on it) but for the above reasons had to switch over to aphrodite late in development. In good news, it wasn't that much to change, the two are fairly easy to port between (except for all the css(...) statements that had to be added everywhere) and we now have a fairly complete understanding of the pros and cons of both.

If you're happy to work through the above issues with us, I would love to use Elemental and our other projects as real-world cases to establish theme-standard and make JSS more capable. This PR is a great start.

Let me know what you think, and I'll ping @nikgraf and @mxstbr for their feedback too - Max is taking a lead role in Keystone's development at the moment which is one of the most significant uses of Elemental at the moment, and I know Nik has interest in this space too with belle and react-future-ui

@kof
Copy link
Member Author

kof commented Apr 23, 2016

  • Regarding server-side rendering, lets discuss it here, I see a number of possible ways to do it.

  • Regarding aphrodites rendering, I also like it. I need to do some benchmarking to ensure it doesn't have bad performance impact, see.

  • Regarding not having a higher order component (react-jss). I use it in production, it works well. I also have some cases where I need to access the component's instance, it works by using a callback similar to ref. Static properties is not something we def. need. In fact we need to try using more functional components and get rid of bad object oriented habits. Blaming react-jss in being a higher order component is kinda naive. We have higher order components everywhere. They are a huge win for react's ecosystem. cc @gaearon

  • We might get a simpler API if we implement something like this, there will be no need in react-jss then.

  • JSS styles have to be generated when the code is interpreted.

    With react-jss CSS is generated when component is about to mount. You can always add more rules to the style sheet, at any stage of components lifecycle. However you need to understand that not everything makes sense to be style sheets. For the most dynamic changes you should use inline styles.

@kof kof changed the title Server-side Rendering, Higher order components, Render-time modifications Feedbackfrom @JedWatson Apr 24, 2016
@kof kof changed the title Feedbackfrom @JedWatson Feedback from @JedWatson Apr 24, 2016
@nathanmarks
Copy link
Member

nathanmarks commented May 5, 2016

@JedWatson @kof

I'm currently working on a new styling system (using jss) for callemall/material-ui to address long standing issues with our basic inline styling implementation.

The solution that I'm working on has the following requirements:

  1. Must be able to use a theme on context
  2. Not require a HoC on every component
  3. Deterministic class names based on the theme
  4. Server side rendering

These stylesheets are intended to be "static" -- dynamic styling should be done via inline styling and/or class switching.

I'll share a demo repo once I've got things whipped into shape. Here's an example of how a component uses the implementation: https://gist.github.com/nathanmarks/ec796ef3dcc59db86eef19acce82e29e

@mr-mig
Copy link

mr-mig commented May 12, 2016

I am wondering why do you need "Deterministic class names based on the theme" at all with jss?

@kof
Copy link
Member Author

kof commented May 12, 2016

@nathanmarks any progress there?

  1. Must be able to use a theme on context
  2. Not require a HoC on every component

I think you will need something like this Lets build a working version for material ui and add it then to react-jss.

  1. Deterministic class names based on the theme

This requires a separate issue with discussion, I am open to get there. Also you might want to consider theme-standard

  1. Server side rendering

-> #221 let me know what you need.

@jacobrask
Copy link

Curious, what is the reason that you don't want to use HoC?

@nathanmarks
Copy link
Member

nathanmarks commented May 12, 2016

@kof

Progress is going well. I am preparing an example to share with the other contributors to get their opinions + feedback.

Once I've presented it to the others and validated the concept (very soon), I'm open to a discussion on we can better support some of the features in jss. Especially the server side rendering features.

I have a working demo right now that isn't using a HoC (albeit it does require some boilerplate in the component for mount/unmount) and uses a customizable theme.

In order to properly support themes + concurrency in node, the styleSheet object created at the root of the component module never holds the resolved class names. Instead, I provide a createStyleSheet() abstraction with a callback that receives the theme as the first argument. There is a module that orchestrates the style resolution using the theme and manages the stylesheets in the DOM. The public API for this module is available on context. It also supports basic server side rendering (no crazy parsing of CSS, it just re-uses the sheets).

My only worry with a theme-standard is that styleguides and design specifications can vary immensely. It would have to be very flexible in order to provide a suitable structure/API for different use cases.

@jacobrask

The project that my experiment is intended for is a UI component library. Part of the reason we're planning on moving to a JS style library is due to performance issues and shortcomings as a result of just using inline styling. There are several features that we would like to use HoCs for, but the problem is that design does not scale well (depending on application size+complexity+setup and # of HoCs).

Let's say you have 3 HoCs on a checkbox component and a table with 40 rows containing a checkbox in each. That's 160 component instances just for the checkboxes. Our checkboxes also have a ripple component which could add another another 80 component instances. Already we're looking at 240 instances just for the checkboxes.

Lastly, this is all hidden from the user as they are just importing <Checkbox />. That's really the key here -- we can't make too many assumptions about what happens in userland and we need our implementation to be as light as possible.

However... one of us is also working on a library that will allow us to compose a single HoC with multiple features without intruding on component logic to save on the # of component instances. But even with this we would need to pass the "HoC" lifecycle functions ourselves.

@kof
Copy link
Member Author

kof commented Jun 23, 2016

deterministic classes generation is released.

@kof
Copy link
Member Author

kof commented Jul 10, 2016

@JedWatson @nathanmarks

To sum up the progress about issues discussed here:

  1. SSR + rehydration - done
  2. React integration without a HOC - points are taken, discussion is ongoing lots of ideas, we will work something out. Just need some time.
  3. Render-time styles rendering ala Aphrodite has one major issue - computed styles might not be applied to the element when it is rendered, which can result in wrong value when you try to access computed value via DOM as well as bring additional recalcs/repaints. For this reason I think we have to handle jss generated styles in a more "static" fashion and use inline styles for "at-render" time stylings. For this reason theming should be also more "static".

@kof kof closed this as completed Jul 10, 2016
@kof
Copy link
Member Author

kof commented Mar 7, 2017

Hi everybody, we are back to this issue, I am struggling to validate the original problem existence, any one ideas? #432 (comment)

@kof
Copy link
Member Author

kof commented Mar 7, 2017

cc @JedWatson do you know what it was originally?

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

No branches or pull requests

4 participants