Material-ui: [4.0.1-beta] Unwanted CSSStyleSheet.insertRule behavior when using styled (passing a function as a param for styles)
- [x] This is not a v0.x issue.
- [x] I have searched the issues of this repository and believe that this is not a duplicate.
Expected Behavior 馃
All styles should be internal in DOM, i.e. the actual CSS should be inlined in the <style>'s tags in the <head>.
Why? Because Prerendering doesn't work otherwise as the styles, instead of being internal, are directly inserted into the CSSStyleSheet, using insertRule (the process is so-called the speedy mode). More info:
Current Behavior 馃槸
Common Header for the following two examples:
import { styled } from "@material-ui/styles"
const component = (props) => <div {...props} /> // just an example
- Works great if using without accessing props, resulting in internal css, rendered in DOM:
export const Div = styled(component)({
width: `100%`,
})
- Uses unwanted CSSStyleSheet.insertRule, not rendering CSS in DOM, directly applying styles to the CSSStyleSheet:
export const Div = styled(component)((props) => ({ // IMPORTANT! Note passing the function
width: `100%`,
}))
Your Environment 馃寧
"dependencies": {
"@material-ui/core": "4.0.0-beta.1",
"@material-ui/styles": "4.0.0-beta.1",
"react": "16.8.6",
"react-dom": "16.8.6"
}
o-alexandrov
All 18 comments
@o-alexandrov I think that it's a tradeoff you have to accept using the style function approach. I doubt there is any solution to it. Now, this is an issue for JSS, it's outside of our scope.
oliviertassinari
on 11 May 2019
@kof probably has more context for this. It's not obvious to me what the issue here is. Maybe not being able to change the styles in the chrome dev tools? Maybe it would be more appropriate to speak to the people responsible for the devtools in this case.
eps1lon
on 11 May 2019
@o-alexandrov what is the ACTUAL problem you have?
kof
on 11 May 2019
@oliviertassinari @eps1lon
I ended up solving the expressed issue with Prerendering not in the most elegant way, but unfortunately anything better is not available yet, by applying the same techniques that are expressed in the:
Steps made:
- Added if statement affecting exclusively Prerendering environment, thus, inlining styles to the
<head>including cases (for example using styled as a function) when JSS inserts styles directly to the StyleSheet interface instead. - Added process to webpack to clean up style declarations from the prerendered HTML files.
@kof
I created a separate issue in JSS to continue there:
o-alexandrov
on 12 May 2019
@eps1lon
Btw, you are correct:
- In Chrome browser, you cannot change style rules directly added to the StyleSheet interface with CSSStyleSheet.inserRule()
- these style rules would be read-only, the classname would be visible as _italic_
However, you can change them in the Firefox browser, for example the following:
- Firefox Quantum: Developer Edition
- you'd have the same experience as with other styles
Still, changing styles doesn't worry me personally at all, as you can apply inline styles to change the behavior, if you are playing with them in the Dev Tools inspecting the element.
o-alexandrov
on 12 May 2019
I still don't understand the problem ... @eps1lon do you?
kof
on 12 May 2019
@o-alexandrov It's unideal, but part of the tradeoff. In practice, I don't think that it matters, editing the top or bottom section is equivalent, each component instance gets his own style.
While with static style, the style is shared between the component instances, you can edit all of them at the same time.
oliviertassinari
on 12 May 2019
Some issue with pre-rendering though it isn't explained what the issue is. Maybe the expectation is that renderToStaticMarkup() would already include the necessary style tags?
eps1lon
on 12 May 2019
@oliviertassinari
I don't think that it matters, editing the top or bottom section is equivalent, each component instance gets his own style.
Please be informed that I typed the exact same thing above, citing myself:
Still, changing styles doesn't worry me personally at all, as you can apply inline styles to change the behavior, if you are playing with them in the Dev Tools inspecting the element.
However, you brought a new good point on why the inconsistency in the process of inserting rules to the StyleSheet worsens the dev experience:
- since the style rules are applied inconsistently and
read-only, in the browser, you can only affect (preview changes on) a single element.
o-alexandrov
on 12 May 2019
@kof @eps1lon
- Inconsistency in insertion of styles is simply unacceptable. The discussion should not go further on this particular aspect of the issue, it is the same thing as following two different patterns to solve the exact same task.
How does it affect prerendering?
- Right now, prerendering is impossible without additional setup to actually inline styles to the
<head>, because look at the picture above, the style rules do NOT exist in the HTML, they are applied to the CSSStyleSheet directly, using the CSSStyleSheet.inserRule().
Let me even screencast it, so you can see that there are no styles added to the DOM, they are added directly to the CSSStyleSheet instance.
o-alexandrov
on 12 May 2019
@o-alexandrov the fact that you can't modify the CSS rule inserted via CSSOM API in chrome dev tools is a known tradeo-off and basically a limitation of chrome dev tools and hopefully will be fixed at some point (is there an issue on chrome tracker?)
kof
on 12 May 2019
@kof
After thinking about changing styles for the group of elements that share the same class name, I actually remember the proper way to do that, you can insert the rule to overcome this issue, see the screencast:
- still, the example above leads to additional time spent on what could have been avoided, if JSS was consistent, following the exact same style inlining to the
<head>, like the following screencast showcases:
o-alexandrov
on 12 May 2019
what could have been avoided
You seem to know more about rendering CSS than I do.
kof
on 12 May 2019
@kof short summary of the whole discussion:
- Why doesn't JSS inline styles in all cases? Or why JSS doesn't do CSSStyleSheet.insertRule() all the time?
- I ask to make the JSS consistent, so the dev experience might become predictable.
Because right now JSS does the following:
Current Behavior
Common Header for the following two examples:
import { styled } from "@material-ui/styles"
const component = (props) => <div {...props} /> // just an example
- Inlines styles to the HTML:
export const Div = styled(component)({
width: `100%`,
})
- Does NOT inline styles to the HTML, instead uses CSSStyleSheet.insertRule:
export const Div = styled(component)((props) => ({ // IMPORTANT! Note passing the function
width: `100%`,
}))
@o-alexandrov What do you mean with prerendering? SSR?
HenriBeck
on 12 May 2019
@HenriBeck please consider to google terminology before asking. However, since I'm asking the community to solve the issue and don't commit spending my time to propose a solution, I will assist in helping to process the terminology:
- Prerendering is not SSR
- When talking about Single Page Applications (SPAs), there are four notable use cases:
- Client-side Rendering (CSR)
- when the body of the HTML the requester receives is empty (or doesn't have the SPA's rendered) and then the SPA renders its code via
ReactDOM.render()(for example in react, since we are in the Material UI issue) - Single file (index.html) SPA when JS is inlined as part of the HTML file as well as the styles, but I will not cover this case as it is irrelevant.
- Server-side rendering (SSR)
- everything stays exactly the same to the CSR, but the body of the initial HTML the requester receives is not empty, instead, it is prerendered by the server and served, already prepared for the requester, so speeding up First Contentful Paint (FCP).
- Finally, Prerendering, instead of spending computing time of the server, the page is prerendered (typically after building as part of webpack, you can use this plugin to prerender SPA), and then it is permanently stored as a saved .HTML file for the page, saving computing costs in the future.
Obviously, all four types are superior to other cases in certain business cases, otherwise, they would have not exist 馃槃
This issue discusses the problem that JSS brings with its inconsistency in:
- inlining styles to
<head>in some cases - skipping inlining to
<head>and applying style rules directly to the CSSStyleSheet instance viainsertRule()in other discussed above cases.
This inconsistency not only worsens the Developer experience while working with JSS, but also worsens the ability to prerender the page attempting to inline all styles to the <head>.
- Instead, we are forced to do extra steps to do that, as described in the message above
o-alexandrov
on 12 May 2019
This is best discussed in an issue in the Jss repo. It seems like the decision was made consciously. Please reference the original issue there so that anybody searching for a solution here is properly forwarded.
It would be interesting to know how other styling solution solve this.
@o-alexandrov I would suggest you change your tone to be less prescriptive. At least for me it's not obvious from this issue why the current approach was chosen. You saying that it should not be chosen sounds like you either know better and don't want to share knowledge or you don't know the full story and act like you do. Either way isn't constructive at all.
eps1lon
on 13 May 2019
@eps1lon
Please reference the original issue there so that anybody searching for a solution here is properly forwarded.
Please be informed that in this post above I actually:
- created a separate issue
- described everything in detail there also
- mentioned that I solved the issue I was having with Prerendering and steps to follow to resolve for others
- I am surprised to see detailed explanations to be met with something other than gratitude.
- Regarding the tone, it is again surprising to see the answer in great detail expressing nuances to a question that could be easily Google'd.
- If I asked someone a question that I could then find as a first link on Google, I would feel deeply sorry to wasting someone's time, it is surprising to see a disagreement on this.
Regarding the:
it's not obvious from this issue why the current approach was chosen. You saying that it should not be chosen sounds like you either know better and don't want to share knowledge or you don't know the full story and act like you do
- I have provided you in this discussion with:
- code samples, such as this benchmark from the mentioned above issue
- extremely detailed explanations above, including screencasts, and explanation of terminology
I am sad to see such misinterpretation of the discussion, and I am sincerely sorry for the tone that you seem to interpreted as _prescriptive_.
On the other hand, I hope you consider to offer more understanding to those, like me, whose English is not the first language and thus would know, that the tone might simply be something you are not used to growing up in a different culture.
o-alexandrov
on 13 May 2019
Related issues
finaiized
路
3Comments
sys13
路
3Comments
ghost
路
3Comments
chris-hinds
路
3Comments
ryanflorence
路
3Comments
Most helpful comment
This is best discussed in an issue in the Jss repo. It seems like the decision was made consciously. Please reference the original issue there so that anybody searching for a solution here is properly forwarded.
It would be interesting to know how other styling solution solve this.
@o-alexandrov I would suggest you change your tone to be less prescriptive. At least for me it's not obvious from this issue why the current approach was chosen. You saying that it should not be chosen sounds like you either know better and don't want to share knowledge or you don't know the full story and act like you do. Either way isn't constructive at all.