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

Aggregate style component for server-side rendering #53

Closed
alexlande opened this Issue Feb 28, 2015 · 12 comments

Comments

Projects
None yet
7 participants
@alexlande
Member

alexlande commented Feb 28, 2015

Problem

Radium media queries and browser states don't work with server-side rendering. This hurts initial render speed (we lose some of the benefits of server-side rendering because the initial render is incorrect), and means that apps using Radium won't display correctly until/if the app is hydrated on the client.

Aside from looking bad, this is an accessibility concern, particularly for browser state events.

Solution?

@rgerstenberger spent some time testing out variations of the Style component to handle this use case. In his tests, he handled :hover, :focus, and :active with style tags in each component, scoped to the parent component's react-id. The performance there was terrible, because the browser had to re-evaluate everything every time it hit a style tag.

The only good solution I can think of is to implement an aggregate style tag (code name: StyleCatcher) at the top of Radium apps. On server-side renders, we would take any media query + browser state styles and drop them there in a way similar to the existing Style component. I'm assuming this will have enough overhead that we won't want to do it on repeated renders on the client-side. If that's not the case, we could replace our existing media query + browser state behavior with the StyleCatcher.

Challenges

  • How do we now if it's a server side render or not (if we need to tell the difference)?
  • How does the interaction between components and StyleCatcher work? When a component calls buildStyles it updates state on StyleCatcher? Probably.
  • More things I'm forgetting?
@alexlande

This comment has been minimized.

Show comment
Hide comment
@alexlande

alexlande Mar 2, 2015

Member

I must be going crazy because my comments seem to be disappearing. Based on some productive Twitter conversation, going to aim to implement server-side media queries and browser states with individual style elements to start, then optimize to a StyleCatcher based on that.

Member

alexlande commented Mar 2, 2015

I must be going crazy because my comments seem to be disappearing. Based on some productive Twitter conversation, going to aim to implement server-side media queries and browser states with individual style elements to start, then optimize to a StyleCatcher based on that.

@lanceharper

This comment has been minimized.

Show comment
Hide comment
@lanceharper

lanceharper Mar 5, 2015

How do we now if it's a server side render or not (if we need to tell the difference)?

Not sure if you're still exploring or already found this, but check out
https://github.com/facebook/react/blob/master/src/vendor/core/ExecutionEnvironment.js#L30

lanceharper commented Mar 5, 2015

How do we now if it's a server side render or not (if we need to tell the difference)?

Not sure if you're still exploring or already found this, but check out
https://github.com/facebook/react/blob/master/src/vendor/core/ExecutionEnvironment.js#L30

@alexlande

This comment has been minimized.

Show comment
Hide comment
@alexlande

alexlande Mar 5, 2015

Member

@lanceharper: That's perfect. Thanks!

Member

alexlande commented Mar 5, 2015

@lanceharper: That's perfect. Thanks!

@alexlande

This comment has been minimized.

Show comment
Hide comment
@alexlande

alexlande Mar 5, 2015

Member

Update on this. We've got the base Style component merged in. I'll be tagging a release later today with that. From here, we need to take care of the following to get server-side Style component rendering:

  • Add media query API to Style (currently it only takes selectors and rules, no support for @ rules)
  • Figure out how the conditional Style components will be handled. Ideally, they should be rendered by StyleResolverMixin transparently if you're on the server. Not sure how feasible that is.
  • Add a standard way to generate dynamic classnames for scoping Style components to their parent (currently it takes an arbitrary "scope" string and it's up to users to decide what that is).

That's the baseline to get us doing proper server-side rendering (but it might not be performant enough, at which point we'll need to expand to the StyleCatcher aggregate component).

Thinking about this now, I'm a little concerned that we'll have to do something like this:

render: function () {
  return (
    <div
      style={this.buildStyles(radStyles)}
      class={this.getStyleScope()}
    >
      {this.props.children}
      {this.getStyleOutlet()}
    </div>
  );
}

In which this.getStyleOutlet() conditionally renders the Style component, and this.getStyleScope() adds a dynamic class name that the Style component uses as a scope selector.

I would really love to not do something that cumbersome for this use case, but it's the first thing to come to mind.

Would also be open to jumping straight to the StyleCatcher if that wouldn't be too painful. We've got to get @ rule rendering in Style first either way (given that StyleCatcher should use the same logic).

/cc @iest

Member

alexlande commented Mar 5, 2015

Update on this. We've got the base Style component merged in. I'll be tagging a release later today with that. From here, we need to take care of the following to get server-side Style component rendering:

  • Add media query API to Style (currently it only takes selectors and rules, no support for @ rules)
  • Figure out how the conditional Style components will be handled. Ideally, they should be rendered by StyleResolverMixin transparently if you're on the server. Not sure how feasible that is.
  • Add a standard way to generate dynamic classnames for scoping Style components to their parent (currently it takes an arbitrary "scope" string and it's up to users to decide what that is).

That's the baseline to get us doing proper server-side rendering (but it might not be performant enough, at which point we'll need to expand to the StyleCatcher aggregate component).

Thinking about this now, I'm a little concerned that we'll have to do something like this:

render: function () {
  return (
    <div
      style={this.buildStyles(radStyles)}
      class={this.getStyleScope()}
    >
      {this.props.children}
      {this.getStyleOutlet()}
    </div>
  );
}

In which this.getStyleOutlet() conditionally renders the Style component, and this.getStyleScope() adds a dynamic class name that the Style component uses as a scope selector.

I would really love to not do something that cumbersome for this use case, but it's the first thing to come to mind.

Would also be open to jumping straight to the StyleCatcher if that wouldn't be too painful. We've got to get @ rule rendering in Style first either way (given that StyleCatcher should use the same logic).

/cc @iest

@jviereck

This comment has been minimized.

Show comment
Hide comment
@jviereck

jviereck commented Apr 2, 2015

\cc me | @jviereck

@david-davidson david-davidson removed their assignment Apr 15, 2015

@jnak

This comment has been minimized.

Show comment
Hide comment
@jnak

jnak May 3, 2015

Any new ideas on this topic?
I would love to see @rgerstenberger benchmark so we can play with it and try to find a solution that does not involve getStyleScope and getStyleOutlet :)

jnak commented May 3, 2015

Any new ideas on this topic?
I would love to see @rgerstenberger benchmark so we can play with it and try to find a solution that does not involve getStyleScope and getStyleOutlet :)

@pocketjoso

This comment has been minimized.

Show comment
Hide comment
@pocketjoso

pocketjoso Jul 12, 2015

Any progress on this feature? Radium looks cool but I need this feature before I can start using it.

For the <style> approach, how do you intend to handle the specificity issue (since you have inline styles)? Make everything !important; i.e. <style> rules trumps all inline ones?

pocketjoso commented Jul 12, 2015

Any progress on this feature? Radium looks cool but I need this feature before I can start using it.

For the <style> approach, how do you intend to handle the specificity issue (since you have inline styles)? Make everything !important; i.e. <style> rules trumps all inline ones?

@ianobermiller

This comment has been minimized.

Show comment
Hide comment
@ianobermiller

ianobermiller Jul 13, 2015

Collaborator

@pocketjoso I think we'd only end up doing this for media queries, and yes !important would be the way it would work. Would media query support alone solve your issues? You wouldn't get hover or active classes, but I'm thinking if you want any interaction you can wait for JS anyway.

Collaborator

ianobermiller commented Jul 13, 2015

@pocketjoso I think we'd only end up doing this for media queries, and yes !important would be the way it would work. Would media query support alone solve your issues? You wouldn't get hover or active classes, but I'm thinking if you want any interaction you can wait for JS anyway.

@pocketjoso

This comment has been minimized.

Show comment
Hide comment
@pocketjoso

pocketjoso Jul 16, 2015

@ianobermiller It would be enough to make Radium viable to test using, yeah. FYI I'm experimenting with ua sniffing to basically go down the route of #146 instead. I'm thinking it might be a better fit for Radium..

pocketjoso commented Jul 16, 2015

@ianobermiller It would be enough to make Radium viable to test using, yeah. FYI I'm experimenting with ua sniffing to basically go down the route of #146 instead. I'm thinking it might be a better fit for Radium..

@ianobermiller

This comment has been minimized.

Show comment
Hide comment
@ianobermiller

ianobermiller Jul 16, 2015

Collaborator

Yeah UA sniffing will get you most of the way there. I wouldn't mind adding UA sniffing to radium core to solve the (likely) 95% use case for media queries to render a different mobile site.

Collaborator

ianobermiller commented Jul 16, 2015

Yeah UA sniffing will get you most of the way there. I wouldn't mind adding UA sniffing to radium core to solve the (likely) 95% use case for media queries to render a different mobile site.

@ianobermiller

This comment has been minimized.

Show comment
Hide comment
@ianobermiller

ianobermiller Nov 12, 2015

Collaborator

So, for an update. As of 0.15.0 Radium uses inline-style-prefixer, which does prefixing via useragent, which means it is identical on the server. Two things are still missing, media queries and keyframes. My plan is to implement StyleCatcher and automatically render it for the root app (isRoot in config). Keyframes will return an object we translate to CSS and inject at render time, same for media queries. Once those tasks are complete, Radium should render without warnings on the server.

Collaborator

ianobermiller commented Nov 12, 2015

So, for an update. As of 0.15.0 Radium uses inline-style-prefixer, which does prefixing via useragent, which means it is identical on the server. Two things are still missing, media queries and keyframes. My plan is to implement StyleCatcher and automatically render it for the root app (isRoot in config). Keyframes will return an object we translate to CSS and inject at render time, same for media queries. Once those tasks are complete, Radium should render without warnings on the server.

@ianobermiller

This comment has been minimized.

Show comment
Hide comment
@ianobermiller

ianobermiller Dec 21, 2015

Collaborator

Closing in favor of #466

Collaborator

ianobermiller commented Dec 21, 2015

Closing in favor of #466

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment