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

[Style] Address server sider rendering issues #3009

Merged
merged 5 commits into from
Jan 22, 2016
Merged

[Style] Address server sider rendering issues #3009

merged 5 commits into from
Jan 22, 2016

Conversation

oliviertassinari
Copy link
Member

This PR introduce many changes.

  • This prevent to be flooded by the warnings when using Material UI fo SSR.
Warning: Material UI: userAgent should be supplied in the muiTheme context
        for server-side rendering.
Warning: Material UI: userAgent should be supplied in the muiTheme context
        for server-side rendering.
Warning: Material UI: userAgent should be supplied in the muiTheme context
        for server-side rendering.
Warning: Material UI: userAgent should be supplied in the muiTheme context
        for server-side rendering.
Warning: Material UI: userAgent should be supplied in the muiTheme context
        for server-side rendering.
  • The prefixer transform is moved to the context.
  • We can now provide a userAgent variable to the muiTheme. We can use the following values:
    • a regular user agent like Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.82 Safari/537.36
    • 'all' to prefix for all user agents
    • false to disable the prefixer
  • Deprecate the usage of autoPrefixer.all()
  • Deprecate the usage of autoPrefixer.getPrefixer()
  • Deprecate the usage of autoPrefixer.getPrefix()

Fix #2530, #2336 and #2316.

@alitaheri
Copy link
Member

Any change we can make this contextual? maybe with theme? I mean globals hurt SSR. so a contextual userAgent can be passed down from the root with each request. the theme object is already on it. so maybe add it to the them?

@oliviertassinari
Copy link
Member Author

Any change we can make this contextual? maybe with theme? I mean globals hurt SSR. so a contextual userAgent can be passed down from the root with each request. the theme object is already on it. so maybe add it to the them?

Yeah, I agree! This PR is WIP, this first commit can be merged like this, but I'm gonne push new commits to address this.

@alitaheri
Copy link
Member

I see. Thanks a lot. This will surely close a huge number of issues 😁 😁

@oliviertassinari oliviertassinari changed the title [Style] Only display the warning once [Style] Start to address server sider rendering issues Jan 21, 2016
@@ -144,13 +144,13 @@ const CircularProgress = React.createClass({
_rotateWrapper(wrapper) {
if (this.props.mode !== 'indeterminate') return;

AutoPrefix.set(wrapper.style, 'transform', 'rotate(0deg)');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why we can't call muiTheme.prefix() rather than calling it on the imported auto-prefix from here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some issue when trying to do so. That's definitely something we should address.

@oliviertassinari
Copy link
Member Author

I have updated the PR, I'm happy with it.

@alitaheri
Copy link
Member

It's good for now 👍 👍 We'll address more issues when we decide on transformers 👍

Feel free to merge 😁

@oliviertassinari
Copy link
Member Author

Awesome, I'm gonna add a section on the doc for the server side rendering before merging.

@oliviertassinari
Copy link
Member Author

Added.

@oliviertassinari oliviertassinari changed the title [Style] Start to address server sider rendering issues [Style] Address server sider rendering issues Jan 22, 2016
- `'all'` to prefix for all user agents
- `false` to disable the prefixer

We rely on the [muiTheme](/#/customization/themes) context to pread the user agent to all of our component.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'pread' / 'spread'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@alitaheri
Copy link
Member

All good 👍 👍 Thanks a lot @oliviertassinari

@oliviertassinari
Copy link
Member Author

@newoga Do you want to have a final look at this PR before merging?

@newoga
Copy link
Contributor

newoga commented Jan 22, 2016

@oliviertassinari Sorry, I looked earlier and I think I was good for now in terms of a first iteration. I'll do one last quick check! 😄

@newoga
Copy link
Contributor

newoga commented Jan 22, 2016

@oliviertassinari @alitaheri Yup, let's get this in 👍

I'm not too familiar with server side rendering with React so I can't comment too much in that regard, but I think this is a good first step. We can look at more approaches to refactoring auto-prefix use in separate PRs.

Part of #2986 motivation was developing an approach for removing auto prefixing from the transformation cycle. Since this PR solves it with a similar but different means, maybe we should hold off on #2986?

I can create a new PR (or modify #2986) that doesn't expose the composition and customization of style transformers but still puts our own version of prepareStyles in context if that's still useful. I think some of the changes like ensuring the prepareStyles (not just ensureDirection) gets called only once is still worth it.

@oliviertassinari
Copy link
Member Author

@newoga Awesome 🎉.

still puts our own version of prepareStyles in context if that's still useful

This PR is making us one step closer to the goal of #2986.
Right now, on the master branch, prepareStyles is working without the muiTheme.
This PR makes it mandatory.
I definitely think that moving everything into the context will make thing more coherent, easier to reason about and be more customizable.

@alitaheri
Copy link
Member

I definitely think that moving everything into the context will make thing more coherent, easier to reason about and be more customizable.

Yes I agree 👍 👍

oliviertassinari added a commit that referenced this pull request Jan 22, 2016
[Style] Address server sider rendering issues
@oliviertassinari oliviertassinari merged commit f1d4b97 into mui:master Jan 22, 2016
@oliviertassinari oliviertassinari deleted the server-side-rendering branch January 22, 2016 19:36
@shilpan
Copy link

shilpan commented Jan 26, 2016

Yay! I have been trying to do server-side rendering and am facing this very same issue. Do you know when this will be released on npm?

@newoga
Copy link
Contributor

newoga commented Jan 26, 2016

@shilpan We're getting ready for 0.14.3 release very soon! #3028

@oliviertassinari
Copy link
Member Author

By the way. I the only advantage of this PR for my project is to remove the warning.
I wasn't providing a global navigator (that display a warning) in order to prefix for all the user agent on the server side. Then, on the client side, I'm only prefixing for the browser user agent.
This allow me to remove the userAgent parameter from my memoized renderToString function (caching).

@thomastvedt
Copy link

I just upgraded to 14.3, and the warning "Material UI: userAgent should be supplied in the muiTheme context for server-side rendering." is back. I had this working before using this hack:

    app.use(function(req, res, next) {
      GLOBAL.navigator = {
        userAgent: req.headers['user-agent']
      }
      next();
    });

How am I supposed to set the userAgent to get SSR working again? (now checksums don't match, and I get the warning, once).

I'm using the decorator in main component App.jsx:

import MyRawTheme from 'theme/my-material-ui.config';
@themeDecorator(ThemeManager.getMuiTheme(MyRawTheme))
export default class App extends Component {

"my-material-ui.config.js":

import Colors from 'material-ui/lib/styles/colors';
import ColorManipulator from 'material-ui/lib/utils/color-manipulator';
import Spacing from 'material-ui/lib/styles/spacing';

export default {
  spacing: Spacing,
  // userAgent: req.headers['user-agent'],
  // userAgent: GLOBAL.navigator.userAgent,
  // userAgent: 'all',
  fontFamily: 'Roboto, sans-serif',
  palette: {
    primary1Color: Colors.blueGrey500,
    primary2Color: Colors.blueGrey700   
  }
};

@newoga
Copy link
Contributor

newoga commented Feb 1, 2016

@tomasztomys Just want to make sure. I noticed your userAgent key is commented in the my-material-ui.config.js. Can did you try uncommenting your userAgent?

@shilpan
Copy link

shilpan commented Feb 1, 2016

I get weird spacing issues in the mismatch error:

(client) ow:0 1px 6px rgba(0, 0, 0, 0.12), 0 1px 
(server) ow:0 1px 6px rgba(0,0,0,0.12),

This is how I set it up.

const muiTheme = getMuiTheme(null, {
    userAgent: 'all',
});
...
export default themeDecorator(muiTheme)(App);

@newoga
Copy link
Contributor

newoga commented Feb 1, 2016

Okay, there's a PR (#3112) that's related to this that I'm going to review soon. I'll see what I find.

@thomastvedt
Copy link

Changing from

@themeDecorator(ThemeManager.getMuiTheme(MyRawTheme))

to

@themeDecorator(ThemeManager.getMuiTheme(MyRawTheme, { userAgent: 'all'}))

fixed it, thank you :-)
That is, I still get the warning from material-ui, but React don't complain anymore (checksum match).

@bravo-kernel
Copy link

For future reference in case somebody else is reading all 1000 posts related to this topic, this worked better for me. Defining my custom theme directly and passing the userAgent as second option.

  • using all instead of false made the UI about 10x slower so using that was not an option
  • throws a browser-side inline-style-prefixer warning
  • still need to figure out how to make the (real) calling client agent available in App.js to kill that inlien-style-prefixer warning
import getMuiTheme from 'material-ui/lib/styles/getMuiTheme';
import themeDecorator from 'material-ui/lib/styles/theme-decorator';
import MuiThemeProvider from 'material-ui/lib/MuiThemeProvider';
import * as Colors from 'material-ui/lib/styles/colors';

const muiTheme = getMuiTheme({
  palette: {
    primary1Color: Colors.indigo500,
    ...etc...
  },
}, {
    userAgent: 'false'
});
return (
  <MuiThemeProvider muiTheme={muiTheme}>
    <AppBar ...etc... />
  </MuiThemeProvider>
)
export default themeDecorator(muiTheme)(MyComponent);

@zannager zannager added the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A way to be able to turn off autoprefixer
8 participants