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

Non-Bug: JSS server rendering produces font families with escaped quote characters. #12924

Closed
2 tasks done
awidjaja opened this issue Sep 18, 2018 · 9 comments
Closed
2 tasks done
Labels
support: Stack Overflow Please ask the community on Stack Overflow

Comments

@awidjaja
Copy link

awidjaja commented Sep 18, 2018

Function createMuiTheme() created the font families with escaped " characters:

fontFamily: "\"Roboto\", \"Helvetica\", \"Arial\", sans-serif" which is converted to " by JSS function SheetsRegistry.toString() instead of ", and this style will be ignore during initial render by browser causing the font to flip.

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

  • output of createMuiTheme():
  fontFamily: '"Roboto", "Helvetica", "Arial", sans-serif'

// or
  fontFamily: "'Roboto', 'Helvetica', 'Arial', sans-serif"
  • style written to index.html
<style id="jss-server-side">html {
    ...
.jss49 {
    ....
    font-family: "Roboto", "Helvetica", "Arial", sans-serif;
}

Current Behavior

  • output of createMuiTheme():
  fontFamily: "\"Roboto\", \"Helvetica\", \"Arial\", sans-serif"
  • style written to index.html
<style id="jss-server-side">html {
    ...
.jss49 {
    ....
    font-family: &quot;Roboto&quot;, &quot;Helvetica&quot;, &quot;Arial&quot;, sans-serif;
}

This style will be ignored by the browser and replaced when the hydration completed with the correct font, causing the screen to flip.

Steps to Reproduce

Please run createMuiTheme() and observe the result.

I suggest this is a bug because material-ui decision to use JSS should mean it should provide what JSS need to work.

  • SSR rendering part
  renderToHtml: (render, Comp, meta) => {
    const sheetsRegistry = new SheetsRegistry() 
    const sheetsManager = new Map() 

    const theme = createMuiTheme()  // <-- this is the problem
\\  the font families were created with escaped quote characters: 
\\ `fontFamily: "\"Roboto\", \"Helvetica\", \"Arial\", sans-serif"`

    const generateClassName = createGenerateClassName()
    const html = render(
      <JssProvider registry={sheetsRegistry} generateClassName={generateClassName}>
        <MuiThemeProvider theme={theme} sheetsManager={sheetsManager}>
          <Comp />
        </MuiThemeProvider>
      </JssProvider>
    )

    meta.jssStyles = sheetsRegistry.toString()  // <-- this will convert escaped quote characters \" to  &quot; instead of "

    return html
  },

-->
Link: N/A

Context

initial font rendered by the client is incorrect, causing the screen to flip.

Your Environment

Tech Version
Material-UI v3.1.0
React 16.5.1
Browser Google Chrome v69.0.3497.100 (64-bit)
TypeScript N/A
etc.
@oliviertassinari oliviertassinari added the support: Stack Overflow Please ask the community on Stack Overflow label Sep 18, 2018
@support
Copy link

support bot commented Sep 18, 2018

👋 Thanks for using Material-UI!

We use the issue tracker exclusively for bug reports and feature requests, however,
this issue appears to be a support request or question. Please ask on StackOverflow where the
community will do their best to help. There is a "material-ui" tag that you can use to tag your
question.

If you would like to link from here to your question on SO, it will help others find it.
If your issues is confirmed as a bug, you are welcome to reopen the issue using the issue template.

@support support bot closed this as completed Sep 18, 2018
@oliviertassinari
Copy link
Member

@awidjaja Tip: you need to use dangerouslySetInnerHTML when injecting the CSS.

@awidjaja
Copy link
Author

@oliviertassinari

I didn't include the part of injecting to the html as I have already isolated the exact location of the disconnect between material-ui and JSS.

But alright, I don't need you to fix it, as this confirm my doubt in the beginning on the use of JSS. Even the installation generated so many errors, missing dependencies, etc. Such a waste of developer time.

I prefer to use emotion, a much better framework in all aspect. Will probably consider mui again in case you ever decide to move to emotion in the future, as long as it doesn't require the consumer to solve integration problem with the framework.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 19, 2018

@awidjaja yes, I'm considering adding an emotion backend to our styling solution. Just need to figure out how it can be done. We took some tradeoff with JSS that makes it hard for people to get it right the first time. I'm looking into improving that. You can use emotion alongside JSS, there is nothing wrong with that. I would love to know more about your pain points. The escape problem one isn't on our side. Many people are doing SSR just fine with JSS.

@awidjaja
Copy link
Author

awidjaja commented Sep 19, 2018

@oliviertassinari

Great to hear that you plan to support emotion, I am sure many people would love it.

Please consider for the createMuiTheme to set a bunch of CSS custom variables instead of an object instance. Ideally one can use the createMuiTheme, a great tool, without having to install JSS dependencies, and then access the CSS custom variables from the custom components, either directly, or through, say, emotion ThemeProvider.

Then you will only need to update material-ui components to get the theme properties from the universal CSS custom variables and free people from having to use a particular css-in-js framework to pass the theme to the components. Then you can focus on building great components than having to make it work with a particular framework.

update: i was able to make it work using regex replace expression, and strangely when I removed the conversion it continued to work. Might be some problem during webpack bundling.

So, no problem with createMuiTheme returning escaped characters, react can handle it. I can now use both Material-UI theme helper and Emotion together.

This is the style injection code that works:

            <style
              id="jss-server-side"
              dangerouslySetInnerHTML={{
                __html: `${renderMeta.jssStyles}`,
              }}
            />

@awidjaja awidjaja changed the title Bug: createMuiTheme function produces font families with escaped quote characters (doesn't work with JSS server rendering) Non-Bug: JSS server rendering produces font families with escaped quote characters. Sep 19, 2018
@oliviertassinari
Copy link
Member

@awidjaja I'm happy to hear it's working for you :). We are using the same approach for the Next.js example.

@oliviertassinari
Copy link
Member

I haven't quite follow your point on the variables. You can inject muiTheme into the emotion theme.

@awidjaja
Copy link
Author

@oliviertassinari

Yes, I am currently injecting muiTheme through emotion theme provider.

Currently each component library brings it's own css-in-js framework (jss, emotion, styled-components, etc.) and theme provider/consumer. Even if you only need to use one component, you'll have to add all the dependencies and add clutter to your react dom tree, and passing/sync the theme. This lead to higher development and maintenance efforts, higher bundle size, higher client's computation and memory footprint.

Imagine that one can have a universal theme of CSS custom variables that are automatically accessible by all components:

  • input
  const theme = {
    palette: {
      primary: '#aabbcc'
    }
  }
  const muiTheme = createMuiTheme(theme)
  • possible output of globally injected CSS variables:
` : root {
  --palette-primary-main: #aabbcc
  --palette-primary-100: ...
  ...other variables
}
  • Developer can then use a single framework to inject the style, e.g. using emotion:
const StyledButton = styled(Button)`
  background-color: var(--palette-primary-main, #ddeeff);
`
  • or override global css variables to for specific component and it's children
const StyledButton2 = styled(StyledButton)`
  --palette-primary-main: var(--palette-accent-200);
  background-color: var(--palette-primary-main, #ddeeff);
`
  • Imagine if all component library (in the future) can accept a universal CSS variable theme, one can do:
  import { Button } from '@material-ui/core'
  ...
  <Button primary>Click Me!</Button>
   // the right CSS variables is auto assigned to the component's style

The CSS variables can also be injected to component using css/sass/css-modules (e.g. bootstrap, bulma. etc.).

Isn't that powerful?

@oliviertassinari
Copy link
Member

You can check out #12827 for the support of CSS variables in the theme. Bulma, Bootstrap, Material-UI are competing libraires. There is little use cases for using two at the same time. I agree on your point regarding CSS-in-JS. In the next style iterations I want to allow people to use different styles backend, JSS, emotion, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
support: Stack Overflow Please ask the community on Stack Overflow
Projects
None yet
Development

No branches or pull requests

2 participants