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

Learnings from JSS #5

Open
kof opened this issue Aug 4, 2016 · 31 comments
Open

Learnings from JSS #5

kof opened this issue Aug 4, 2016 · 31 comments

Comments

@kof
Copy link

kof commented Aug 4, 2016

Hi Nathan, we have been talking a lot about JSS and you seemed to be prototyping a material-ui integration with JSS, however you ended up with an own similar implementation. Can you share the learnings you have done and especially the reasons which stopped you from using jss as is?

@nathanmarks
Copy link
Owner

nathanmarks commented Aug 4, 2016

@kof Definitely, this project started out very much as an experiment (and still is somewhat), material-ui has some specific needs that I tried to address here. I'll write a more detailed response in the next few days.

@kof
Copy link
Author

kof commented Aug 4, 2016

I am interested in adding what is needed, to allow material-ui use it. I believe material-ui requirements can't be so different. A big deal breaker was the SSR (undeterministic id's) which is changed now.

@kof
Copy link
Author

kof commented Aug 4, 2016

I think the entire css in js community would profit if we combine our forces. The amount of work is sooo huge that nobody can do that all alone. I have recently added a roadmap to jss https://github.com/cssinjs/jss#roadmap which reflects the biggest challenges in the project.

@nathanmarks
Copy link
Owner

nathanmarks commented Aug 10, 2016

@kof I agree that it would be great to pool effort towards a project. I'm still intending on writing an overview of the requirements/difficulties/findings I encountered trying to find a solution for material ui with jss in particular. They weren't all unsolvable with jss, it was the closest I got out of the libraries I had tried using. I also tried aphrodite and another one before jss... the name escapes me though right now.

One or two of the requirements might not be considered a "best fit" for css in js, but when the library (material-ui) has a diverse user-base certain features are necessary to provide a good developer experience all around (also related to the current developer tooling available). They could be made possible with the right options and/or plugin API. I think I got around one of them with jss by mis-using a feature (global named selectors).

I'm in my last week at my current job and things are suddenly very busy, but I'll write out more on the weekend. I'm off next week.

@nathanmarks
Copy link
Owner

@kof

There are a few difficult issues that I encountered trying to rig up a solution for material-ui using jss (and a couple of other libs).

An important thing to keep in mind while reading this is that the requirements of material-ui are different from a lot of other use cases because at the end of the day we need to provide a usable library API for a diverse group of users. In contrast, it's much easier to work around the difficulties of css in JS when it is being used internally in a system where you and your team control the front-end and can make decisions on sacrifices, workarounds, "gotchas", etc....

I think you'll find that some of the design decisions in stylishly help cater towards a specific use case, and are not necessarily "the best" way to do things for CSS in JS if you're looking at a blank slate.

We really want to make CSS in JS work for us and our users.

createStyleSheet and theming

material-ui uses a dynamic, js based theming system which sits on context in the user's React application.

Whatever const styleSheet = createStyleSheet(...) returns needs to be a stateless object (server side concurrency) that can be used to generate stylesheets with any theme variables. This immediately adds a requirement to create an abstraction around jss in order to be able to create stylesheets at render using the theme currently on context with a user friendly API.

Right now, stylishly accomplishes this by instead creating an object with a callback property, which the styleManager then uses when stylesheets are rendered by supplying the theme object as a first argument to the callback function.

Classnames/selectors

We have a lot of users who use regular CSS. They expect to be able to inspect their application, copy a className and override styles.

This means a couple of things:

A) Selectors need to be identifiable and ideally have some indication of the component they are from
B) If we change a property value in a rule internally in the library, the selector text needs to remain the same, meaning our appended hash does not represent the styles for the rule.

B is likely very different to how a lot of CSS in JS libraries are handling selectors, but it's fairly central to being able to provide the type of API that material-ui users are expecting.

In order to differentiate selectors there are several mechanisms:

  1. "sheets" are created with a name argument (usually the component name, as those should be unique) and warnings are displayed if you render multiple sheets with the same name.

  2. Selectors all have a hash appended to them which is a unique identifier for the current theme. This is either a hash of the theme object or a user defined string, (eg: button__root--abc123).

Specificity, passing classes, etc.

This is a big challenge for all CSS in JS libraries. It's also fairly critical to provide decent override mechanisms for users when using CSS in JS inside a library.

A user should be able to just pass their own class and as long as the selector is of the same specificity, it should override it. Users of the library should also be able to leverage the same CSS in JS solution built into material-ui to create their styles without difficulty overriding at the same level specificity.

There are several design decisions in place to help accomodate this without too much mess with something like individual stylesheet priority if you have a huge number of stylesheets (1 per component, for example).

By default, there is only a single <style> element rendered to the DOM when using stylishly, all rendered sheets have their styles appended to the content of that element. This allows us to render the library styles in their own "group" in a style tag which is above the user styles. I also added the ability to pre-place the style tags for your "groups" so you can arrange them however you want in the other items in your <head>. This is a lot simpler than a granular per-sheet priority system, but still provides the ability to override any of the styles coming from the library itself.

This still doesn't solve specificity problems within a single stylesheet element ("group") though, and adding a priority system for individual "sheets" is not something I wanted to do. Instead I've added support for descendant selectors, this allows you to leverage CSS specificity to pass a classname into a nested component and override the styles without worrying about style order. Sure, it won't help if you're trying to override the root node, but it does help with a lot of use cases.


I have more to add, but wanted to reply with something to get the discussion going!

@kof
Copy link
Author

kof commented Aug 18, 2016

createStyleSheet and theming

Yes theming for react is a missing part in react-jss, however jss itself is ready, you can do a lot of things with it, for e.g. I am experimenting right now with aphrodisiac

Selectors need to be identifiable and ideally have some indication of the component they are from

Yes, one way is to have component name in every selector, another way is to have component name in a style tag attribute. As we have style tag per component. The last one is a matter of passing meta property when creating sheets. Also when people use react, they can use it's dev tool to identify the component. This has been no problem so far.

If we change a property value in a rule internally in the library, the selector text needs to remain the same, meaning our appended hash does not represent the styles for the rule.

Need more context to understand use cases, don't get it. Are you talking about at-runtime rule style modifications which should not affect the generated selector? If yes it is already this way.

Specificity, passing classes

It needs to be ensured that users style tag or link tag is later on in the dom. This might require a small change in jss but not a big deal. As long jss generated selectors are flat (no nesting), they are easily overwritable.

Being able to choose between using classes or styles objects to theme a component is exactly what I am trying to accomplish by theme-standard which needs a lot of documentation as of now, @rofrischmann works on adding examples and validator library, the last one is needed to ensure the theme passed by user is what component expects and provide meaningful errors.

huge number of stylesheets

For this, theme-standard has subthemes, which allows to nest themes same way we nest components.

This allows us to render the library styles in their own "group" in a style tag which is above the user styles.

This can be achieved with style-per-component approach as well by inserting generated styles one after another and putting the first one before any link/style appearance in the head.

Also one style for everybody can be achieved, see aphrodisiac

pre-place the style tags for your "groups"

You mean the passing style element when creating sheet to the renderer, so that it takes your existing one which is placed to ensure specificity, right? You and me added this to jss some time ago.

CSS specificity

The general rule I can define for sure to avoid this kind of problems is: "Don't use multiple classes on one element, especially if they are from different sources.".

As the library requirements are to provide class based theming as well as style object based. You run into specificity conflicts if

  1. Lib's sheets are not ensured rendered before the users sheets.
  2. Lib's selectors have a higher specificity than a simple class selector. Which means don't use nesting, important , child or sibling selectors.

I think both points can be achieved.

All in all I haven't seen so far big jss specific problems. It is all around react integration and theming. I think you need to distinguish the rendering part and the component level part.

@kof
Copy link
Author

kof commented Aug 18, 2016

Created this issue for specificity problem.

@cvle
Copy link

cvle commented Aug 18, 2016

I'd be happy to see you both join forces, instead of introducing yet another styling solution. Both projects are just too similar. I'm currently using JSS to style my components and actually I decided to go the opposite way and defined a priority for each stylesheet. So far it went straight forward and wasn't "messy". Let me tell you how I do it, maybe it can give you some inspiration.

All my stylesheets are registered into a theme object and receive an implicit priority according to the order of when they were registered. I use a custom DOM renderer that obeys to the priorities when rendering the stylesheets. This is pretty simple and has so far solved all styling issues I encountered with other css based styling solutions.

I used react-toolbox before which is based on CSS-Modules. They use descendant selectors in their styles which has caused me headaches when trying to customize the style. This and the css specificity problem has lead me to drop the project. I highly discourage the use of descendant selectors, they are difficult to overwrite and causes confusion.

@kof
Copy link
Author

kof commented Aug 18, 2016

@cvle how to you define priorities for your sheets? Any code samples?

@kof
Copy link
Author

kof commented Aug 18, 2016

@cvle lets move priority discussion to a separate issue in jss project, because this thread should be focused. I think I could add something to jss to allow manual priority definition.

@kof
Copy link
Author

kof commented Aug 18, 2016

Regarding descendant, sibling and other kinds of nested selectors: they are all considered harmful. They break components encapsulation. The only way they can be used correctly is defining margins and positions/layouting on a child component, which should not be of concern of the child. But lets not go deeper into this.

@nathanmarks
Copy link
Owner

nathanmarks commented Aug 18, 2016

@kof

All in all I haven't seen so far big jss specific problems. It is all around react integration and theming. I think you need to distinguish the rendering part and the component level part.

You're 100% correct, although I was having to use it in an unnatural way before to achieve certain goals. jss was by far the best solution that I tried using, which is why I created the prototype with it.

Quick note re the descendant selectors:

I completely agree they should be used sparingly, but I think there are more use cases where the extra specificity can make something work much more easily.

For example, I'm yet to find a better solution for handling use cases where something such as a color needs to be changed to fit in with the background. Let's say you're a user, you create yourself an icon component which has a grey color and you decide to place it on a dark blue toolbar and want to color the icon white instead (or any colour combination for that matter). What would you suggest as the className based solution here? All the obvious methods involve some sort of broken encapsulation because of how CSS is designed.


You mean the passing style element when creating sheet to the renderer, so that it takes your existing one which is placed to ensure specificity, right? You and me added this to jss some time ago.

Yes, but that was to target for server side rendering reconciliation with the sheets, it doesn't help keep component styles grouped together when more sheets start getting rendered. Or are you suggesting to use only 1 jss sheet as an implementation detail behind custom react bindings that have the react based theming features material-ui needs? This is also a possibility.

Yes, one way is to have component name in every selector, another way is to have component name in a style tag attribute. As we have style tag per component. The last one is a matter of passing meta property when creating sheets. Also when people use react, they can use it's dev tool to identify the component. This has been no problem so far.

I know that here we are mixing the concerns of components + rendering. But this is exactly what I'm talking about when I say that there are design decisions that are catering towards material-ui users. I'm talking about people writing their own CSS and targeting classes that are generated by the material-ui components.

There is an option here to get rid of these mixed concerns and stipulations about className generation: make material-ui component classNames an implementation detail instead of a public API. @oliviertassinari What are your thoughts on this? I think this is one of the areas that stylishly has made some sacrifices

Need more context to understand use cases, don't get it. Are you talking about at-runtime rule style modifications which should not affect the generated selector? If yes it is already this way.

Nope, was talking about actual changes to the rule in the component file. This ties in directly with what I was saying above about providing an API that is friendly for users that use CSS in a more basic fashion.

It's probably confusing because it isn't the route that makes the most sense outside of this use case. I'm really starting to think that we should completely drop this idea and keep selectors as an implementation detail. For the future I definitely think the group of users that the design decision helps will shrink.


Bottom Line

@kof The attempt to make selectors a public API is holding the styling solution material-ui back in some ways and preventing a better separation of concerns between components and rendering. It is an attempt to create a medium-term "bridge" for users who are still doing CSS in a less advanced manner.

If we drop that requirement, it's going to be a lot easier to use jss.

@nathanmarks
Copy link
Owner

@kof

Do you see the ideal way for material-ui to use jss is as a rendering engine behind some additional features?

@kof
Copy link
Author

kof commented Aug 18, 2016

Let's say you're a user, you create yourself an icon component which has a grey color and you decide to place it on a dark blue toolbar and want to color the icon white instead (or any colour combination for that matter). What would you suggest as the className based solution here?

Icon needs to accept a class name which can overwrite props from the system styles on the proper element. However this gives to user ability to overwrite anything else on that element, so yes this is not ideal and can't be controlled, but will certainly work for the user with some caution.

Yes, but that was to target for server side rendering reconciliation with the sheets, it doesn't help keep component styles grouped together when more sheets start getting rendered. Or are you suggesting to use only 1 jss sheet as an implementation detail behind custom react bindings that have the react based theming features material-ui needs? This is also a possibility.

I would rather make renderer place jss styles one after another in some specified order, so that they are grouped. 1 sheet is also a possible way, however I like the 1:1 component-sheet relation, it is easy to see in the dom the entire sheet for one component.

I'm talking about people writing their own CSS and targeting classes that are generated by the material-ui components.

Oh I didn't realize that. I missed the point that you actually want to provide components with global classes. I thought the way you want to allow use regular css with components is to let user pass class names to components, which are then applied. This would fit also css-modules.

const theme = {
  classes: {
    someElement: '.my-very-special-selector'
  }
}

I don't know if we can convince the material-ui community that global classes are evil. But in any case user doesn't need to access generated classes at all. If you want to provide global classes additionally, you can put them additionally to generated classes on the element. This is even easier to achieve because user doesn't need to pass classnames maps. But this is again ... evil.

<button className={`${generatedClass} myButton`}>My button</button>

Do you see the ideal way for material-ui to use jss is as a rendering engine behind some additional features?

Totally. I think the component integration you have made will be perfectly able to use jss as a rendering engine.

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Aug 18, 2016

There is an option here to get rid of these mixed concerns and stipulations about className generation: make material-ui component classNames an implementation detail instead of a public API. @oliviertassinari What are your thoughts on this? I think this is one of the areas that stylishly has made some sacrifices

I pretty much agree with @kof. I can't see why we would need to expose global class names at all.
As long as users can control that their specific class names have a higher priority over the material-ui rules we are fine.

with components is to let user pass class names to components

That would pretty close to how material-ui users override the inline style, thanks to style and nested style properties.

If you want to provide global classes additionally, you can put them additionally to generated classes on the element.

I'm not sure it's the best approach, but yes, that's also something we could do 🙈 .

@nathanmarks
Copy link
Owner

@kof

The more I consider it, the more I think that making the material-ui classes an implementation detail and not a public API is a better idea, it allows for a lot more flexibility in the style implementation. (Not to mention potential breaking changes in the library!)

All components must be able to accept className from users. I think the best solution is to leave it up to the users to pass in their classNames. As you said, it's probably going to be difficult to convince them. Especially those who are accustomed to doing development that way with something like bootstrap, as not having a global class to target out of the box is rather inconvenient for that type of usage. It would require the user to pass that className into every instance of the component themselves.

Honestly though, it would still be a significant improvements for CSS users over what we have right now: inline styles.

I would rather make renderer place jss styles one after another in some specified order, so that they are grouped. 1 sheet is also a possible way, however I like the 1:1 component-sheet relation, it is easy to see in the dom the entire sheet for one component.

Are you thinking a basic priority system? IIRC you were not in favor of one previously, what has changed your mind?

Totally. I think the component integration you have made will be perfectly able to use jss as a rendering engine.

By component integration -- are you referring to the react/theme/context setup with styleManager?

@cvle
Copy link

cvle commented Aug 19, 2016

@nathanmarks I'm curious to know what you see as the downsides when using priority based rendering. A components author would have complete control over which styles overrides which. User styles can be given higher priority to allow easy overriding. I don't know material-ui in detail but Isn't your theme object aware of all your component styles anyway? It would be easy to let it manage the priorities as well.

@nathanmarks
Copy link
Owner

@cyle at first I wasn't certain but I've changed my mind about it. it's worth it given the nature of CSS and could offer a lot of power. In the end we have to use the features CSS offers to help solve the problems at hand.

@nathanmarks
Copy link
Owner

@cvle I atted the wrong person above! Also, I am using sheet ordering currently in this lib too in a basic way, not using a priority system. But I'm sold on the priority system idea for JSS. Would help solve a lot of the problems with ordering and specificity without having to use more complex selectors to begin with.

@cvle
Copy link

cvle commented Aug 20, 2016

Related issue: cssinjs/jss#290

@cvle
Copy link

cvle commented Aug 20, 2016

@nathanmarks Now you have taken my strongest reasons for writing my own complete set of material based components, given that now we are taking a very similar approach :-) (except that I write in typescript). One thing I'm trying to solve atm is to easily customize the style of deeply nested components. Instead of passing multiple stylesheet references as props, I'm playing around with uniquely prefixing the css classes of every component, so I can pass a single user stylesheet to customize the style of every nested component. Additionally I'll play around with descendants selectors to customize child components of the same class with different styles (thanks for that idea). Hoping to spark some ideas for you too.

If you include a good solution for customizing deeply nested components, and include auto- mounting/unmounting of stylesheets, I'm totally sold for the next version of material-ui ;-)

@cyle
Copy link

cyle commented Aug 20, 2016

lol i don't know what's going on here but i'll just cc @kmck on this guy because this repo looks familiar

@nathanmarks
Copy link
Owner

nathanmarks commented Aug 20, 2016

@cvle

In your opinion, what are the biggest benefits of unmounting stylesheets when components are unmounted?

Originally I was a fan of the technique, but I started to wonder if the potential benefits are worth the risk.

Sure, we end up with a little less CSS in the DOM once the component is unmounted, but what is the real performance gain from that? On the other hand, the risk is that you have a component that comes in and out frequently and you are repeatedly adding and removing the entire sheet from the DOM, meaning the browser has to rebuild the CSSOM (unless I am misunderstanding something).

@nathanmarks
Copy link
Owner

@cyle I mixed you up on my phone with @cvle 😄

@nathanmarks
Copy link
Owner

@cvle By the way, there will be less nested components on next and more composition too.

@kmck
Copy link

kmck commented Aug 20, 2016

oh hey cool i was randomly tagged into this. neat project! i made a webpack loader that does kind of a similar thing, but i'm optimizing for a different use case (i think). here it is, in case anyone is curious: https://github.com/kmck/style-component-loader

@cvle
Copy link

cvle commented Aug 21, 2016

@nathanmarks You have good arguments, appreciated.

@kof
Copy link
Author

kof commented Aug 21, 2016

I would rather make renderer place jss styles one after another in some specified order, so that they are grouped. 1 sheet is also a possible way, however I like the 1:1 component-sheet relation, it is easy to see in the dom the entire sheet for one component.

Are you thinking a basic priority system? IIRC you were not in favor of one previously, what has changed your mind?

I didn't have the use case where user wants to overwrite jss styles from css by using global classes.

By component integration -- are you referring to the react/theme/context setup with styleManager?

yes

In your opinion, what are the biggest benefits of unmounting stylesheets when components are unmounted?

My original assumption was that more css should slow down elements creation, as logically DOM needs to run every element agains all registered selectors. However I didn't came to prove that by real benchmarks, I don't even know how to accurately measure this. Here is the corresponding issue, any help appreciated.

On the other hand, the risk is that you have a component that comes in and out frequently

Do you have a use case?

@nathanmarks
Copy link
Owner

@kof

I started on a prototype implementation using jss for the back-end, I already have this running locally in my material-ui next branch: https://github.com/nathanmarks/jss-theme-reactor

@kof
Copy link
Author

kof commented Aug 21, 2016

On the other hand, the risk is that you have a component that comes in and out frequently

Actually even in this case, sheet will not be rerendered, just attached/detached the style node ... most probably there is almost no overhead.

@nathanmarks
Copy link
Owner

@kof I think with either technique, only edge cases are going to have any sort of real performance overhead.

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

6 participants