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

Unable to use this in isomorphic apps #748

Closed
super-cache-money opened this issue Jun 4, 2015 · 24 comments
Closed

Unable to use this in isomorphic apps #748

super-cache-money opened this issue Jun 4, 2015 · 24 comments
Labels
duplicate This issue or pull request already exists package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.

Comments

@super-cache-money
Copy link
Contributor

Since Modernizr only inserts the relevant browser prefix on the client render, there's a mismatch with the unprefixed server side render. As alluded to in #591 , this causes the following warning in react:

Warning: React attempted to reuse markup in a container but the checksum was invalid. This generally means that you are using server rendering and the markup generated on the server was not what the client was expecting. React injected new markup to compensate which works but you have lost many of the benefits of server rendering. Instead, figure out why the markup being generated is different on the client or server...

I don't see any easy fixes for this, and it's quite a biggie, which is saddening. Obviously getting this whole module to move away from inline styles is a no-go.

Pure speculation: maybe it's possible to patch the way react handles styles, so that when a component is supplied with style={{'transition': 'all 1s'}}, it's prefixed at the same time it's rendered to HTML, so it becomes:

 style="-webkit-transition: all 1s; transition: all 1s;" 

instead of simply

style="transition: all 1s;"
@MrOrz
Copy link

MrOrz commented Jun 4, 2015

Currently I thought of 3 ways to fix this:

  1. Provide autoprefixer with user-agent data of the request on the server side, as mentioned in Vendor prefix FormidableLabs/radium#128 (comment) .
  2. Defer the autoprefixing process to componentDidMount, which happens after checksum validation. However, since the componentDidMount is invoked after initial render, it will probably suffer from FOUC problem.
  3. Make autoprefixer generate all possible browser prefixes in both server & client. It won't have FOUC issue and all CSS preprocessors have been doing this for years.

@tomchentw
Copy link

Vote up for 3 by @MrOrz !

@wenbing
Copy link

wenbing commented Jun 4, 2015

vote up for 3 👍

@super-cache-money
Copy link
Contributor Author

The problem with 3, is that sometimes you need one css property to map to several different values. For example, prefixing

{ display: 'flex'}

needs to give:

{ 
  display: 'WebkitBox',
  display: 'WebkitFlex',
  display: 'msFlexbox',
  display: 'flex'
}

which can't be represented in a react style object.

Maybe we could submit a PR to react, so that components can accept styles like:

{
  display: [ 'WebkitBox', 'WebkitFlex', 'msFlexbox', 'flex'],
  color: 'red'
}

@pomerantsev
Copy link
Contributor

I would like to chime in here.
Is that warning really a problem?
Although it would be nice to have the same markup generated client- and server-side, it has drawbacks (as @wmertens pointed out).
Maybe it's a good idea to handle this case in React itself - it might be a common issue.

@MrOrz
Copy link

MrOrz commented Jun 5, 2015

@pomerantsev Yeah I agree with you that the problem is not just for material-ui; the whole React community is still exploring how to achieve the holy grail of modular, inline CSS.

The checksum warning is not a big deal in most cases. However, it does indicate that React is rewriting the whole DOM under the mount node ( https://github.com/facebook/react/blob/master/src/renderers/dom/client/ReactMount.js#L884 ). Personally I think it is more expensive than server-side prefixing, which involves just simple string catenation.

As for which prefixes to add, maintaining an up-to-date prefix database indeed requires lots of hard work. But autoprefixer does that heavy lifting for us, and material-ui is already using autoprefixer.

@wmertens
Copy link

wmertens commented Jun 5, 2015

That react rewrite only happens once, right? Is that such a big deal? Isomorphic apps provide fast initial render speed and SEO, and the local rewrite is invisible to the user.

Alternatively, the prefixer could adjust the checksums.

Finally, server-side rendering could use auto-prefixer internally, probably it's not that hard to make sure client bundles do not include auto-prefixer.

@jdsharp
Copy link

jdsharp commented Jun 5, 2015

the local rewrite is invisible to the user

That's not the problem with shipping server rendered markup, the issue is you've generated the markup server side and react in the browser is unable to use any of it. So you've transferred a payload that provides no benefit.

@troutowicz
Copy link
Contributor

Can you give an example on how you are rendering your code server side? Are you sure inline css is the culprit? I use inline CSS in my geoshare app and do not receive checksum errors. The only time I ever received the checksum error was when I was not passing matching props/state to both the server and client.

@hai-cea
Copy link
Member

hai-cea commented Jun 25, 2015

Related to #705

@x1a0
Copy link

x1a0 commented Jul 1, 2015

I am having same problem and thanks to this issue now I understand why this happens. However regardless of how to make autoprefixing work on server side, my question is why material-ui uses inline CSS? I am pretty sure there are proper reasons but could not find any docs about that.

Thanks!

@wmertens
Copy link

wmertens commented Jul 1, 2015

A solution would be to create css just for the things that need prefixing,
like with single classes for each thing.

On Wed, Jul 1, 2015, 09:47 Xiao Zhang notifications@github.com wrote:

I am having same problem and thanks to this issue now I understand why
this happens. However regardless of how to make autoprefixing work on
server side, my question is why material-ui uses inline CSS? I am pretty
sure there are proper reasons but could not find any docs about that.

Thanks!


Reply to this email directly or view it on GitHub
#748 (comment)
.

Wout.
(typed on mobile, excuse terseness)

@tleunen
Copy link

tleunen commented Jul 1, 2015

Not sure it was a good thing to remove all css for inline styles...

It makes the DOM really hard to read, and it's impossible to override styles with our custom CSS.
I would really prefer that MUI goes back to a CSS file

@AVVS
Copy link

AVVS commented Jul 1, 2015

There is a theme manager you can use - its pretty easy

On Jul 1, 2015, at 12:27 PM, Tommy notifications@github.com wrote:

Not sure it was a good thing to remove all css for inline styles...

It makes the DOM really hard to read, and it's impossible to override styles with our custom CSS.
I would really prefer that MUI goes back to a CSS file


Reply to this email directly or view it on GitHub #748 (comment).

@MrOrz
Copy link

MrOrz commented Jul 2, 2015

@x1a0 @tleunen As for me using inline styles or CSS is like a matter of taste. @callemall just took a stand.

Some reasons for & against using inline styles are aggregated in this awesome css-tricks article:
https://css-tricks.com/the-debate-around-do-we-even-need-css-anymore/

And here is the discussion about using inline styles in material-ui: #30

@x1a0
Copy link

x1a0 commented Jul 2, 2015

@callemall @MrOrz In our project, we are using https://github.com/twbs/bootstrap-sass with https://github.com/webpack/css-loader. This combination gives us a clean global CSS environment. For example, you can create a file called user-project.scss along with your UserProfile.jsx which contains:

:local(.userProfile) {
    @import '/path/to/bootstrap/buttons';

    // overriding bootstrap's css here if you need
    ...
}

Then in the generated CSS, you will have Bootstrap's button classes defined under your "local" class, something like:

.user_profile-XYZ .btn { ... }

In your React component you can just write:

import Styles from './user-profile.scss';
render() {
    return (
        <div className={Styles.userProfile}>
            <div className="btn">This is a button with Bootstrap style</div>
        </div>
    );
}

In this way we can keep CSS of material-ui in CSS files while still easy to customize the theme.
I think it would be nice if material-ui can be used in the same way :)

@AVVS
Copy link

AVVS commented Jul 2, 2015

imo this is pretty complicated and too opinionated

On Jul 2, 2015, at 12:24 AM, Xiao Zhang notifications@github.com wrote:

@callemall https://github.com/callemall @MrOrz https://github.com/MrOrz In our project, we are using https://github.com/twbs/bootstrap-sass https://github.com/twbs/bootstrap-sass with https://github.com/webpack/css-loader https://github.com/webpack/css-loader. This combination gives us a clean global CSS environment. For example, you can create a file called user-project.scss along with your UserProfile.jsx which contains:

:local(.userProfile) {
@import '/path/to/bootstrap/buttons';

// overriding bootstrap's css here if you need
...

}
Then in the generated CSS, you will have Bootstrap's button classes defined under your "local" class, something like:

.user_profile-XYZ .btn { ... }
In your React component you can just write:

import Styles from './user-profile.scss';
render() {
return (


This is a button with Bootstrap style


);
}
I think it would be nice if material-ui can be used in the same way :)


Reply to this email directly or view it on GitHub #748 (comment).

@imouto1994
Copy link

@hai-cea Is there any new solution for this problem yet ?
Currently, a possible solution I can think is to allow user to pass a browser prop to the component so we can reuse the existing prefixer with some changes in logic. The default props should be the detected browser using Modernirz

@kriscarle
Copy link

In case it helps, I got React isomorphic working with this https://github.com/react-materialize/react-materialize which is using materialize http://materializecss.com/

@manuelmazzuola
Copy link

Any updates ?

@hourliert
Copy link

👍

@TheUltDev
Copy link
Contributor

I have a solution working, I'll submit a PR today.

@justrhysism
Copy link

News?

@oliviertassinari
Copy link
Member

This is a duplicate of #705.
I'm gonna close this thread.

@zannager zannager added duplicate This issue or pull request already exists package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Development

No branches or pull requests