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

injectGlobal leaks accross multiple requests in SSR #573

Closed
azizhk opened this issue Feb 15, 2018 · 7 comments
Closed

injectGlobal leaks accross multiple requests in SSR #573

azizhk opened this issue Feb 15, 2018 · 7 comments

Comments

@azizhk
Copy link
Contributor

azizhk commented Feb 15, 2018

  • emotion version: 9.0.1
  • react version: 16.0.0

Relevant code.

injectGlobal`
  html, body {
    font-size: ${[22, 24, 26][Math.floor(Math.random() * 3)]}px;
  }
`

What you did:
What I did was something bad just to break and create an example.
I am picking a random number from 22, 24 and 26 and setting the font-size of html,body to be it.

What happened:
On my first request I got the font with 22px and then my page specific style.
On my second request I got the html, body {font-size: 24px} appended after the first request's styles.
On my third request onwards I got html, body {font-size: 26px} appended after the second request's styles.

Reproduction:
Clone this:
https://github.com/zeit/next.js/tree/master/examples/with-emotion
and modify:
https://github.com/zeit/next.js/blob/master/examples/with-emotion/pages/index.js
to randomize font-size.

Problem description:
If I want to use a third party component in one page, I would injectGlobal it and then it would come up in other places.

Suggested solution:
Something like a wrapper around ReactDOM.renderToString

const styles = extractCritical(() => {
  return ReactDOM.renderToString(<Page />)
})

Where it only gives me the styles injected in the render of
And extractCritical automatically resets the global styles before and after the function call.

@azizhk
Copy link
Contributor Author

azizhk commented Feb 15, 2018

Oh so I found out flush mentioned in #491 (comment) but could not find its documentation.

Is it not part of the spec or bad practice?
Or do you want documentation written for it?
How to tell emotion that these styles are going to be used on every page and some styles are for this third party component to be added globally only on the current page?

@tkh44
Copy link
Member

tkh44 commented Feb 16, 2018

Try moving the injectGlobal styles out of the component like this PR shows @azizhk

https://github.com/zeit/next.js/pull/3821/files

@azizhk
Copy link
Contributor Author

azizhk commented Feb 16, 2018

How do you recommend we work with third party components which have their own stylesheets?
I have react-select being used on a few pages and am importing it using injectGlobal.
But then it got added in all pages.

@azizhk
Copy link
Contributor Author

azizhk commented Feb 16, 2018

Also do you recommend anything for development when we are modifying the values of injectGlobal?
Like every change that I make to injectGlobal value keeps getting appended to the styles.

screen shot 2018-02-16 at 12 59 47 pm

/* HTML Beautified for screenshot */

@azizhk
Copy link
Contributor Author

azizhk commented Feb 16, 2018

realized that I cannot use flush along with https://github.com/zeit/next.js/pull/3821/files
After flush, it is needed that emotion functions be called again, will not work for hoisted calls.

component.js

import styled from 'react-emotion'

const WhiteDiv = styled.div`
  background-color: white;
`
export default function () {
  return (
    <WhiteDiv>This should be white</WhiteDiv>
  )
}
import WhiteDiv from './component.js'
import { flush } from 'emotion'

function render () {
  // lets say first request you render
  const { html, ids, css } = extractCritical(renderToString(<WhiteDiv />))
  // before second request you flush
  flush()
  // second request comes in
  const { html, ids, css } = extractCritical(renderToString(<WhiteDiv />))
  // now this type css will not have the proper styles because
  // WhiteDiv was hoisted and styled.div was not called again.
}

@emmatown
Copy link
Member

@azizhk There are two ways I think you could solve this.

  1. Don't use injectGlobal, use css and react-helmet to add the class to the html and body element. (You should probably do this unless you're cool with using experimental stuff)
  2. Use @emotion/global(you'll also need to install @emotion/core and @emotion/css) which will render the styles directly in the react tree, avoiding this whole problem. (the warning at the top of the README is mainly warning about breaking changes rather than bugs in the implementation, it's pretty well tested but you should lock the version) It depends on react@16.3(right now just the alpha) so you can use the alpha or install create-react-context and polyfill the new context API like this.
// put this in a separate file and make it the first import in your app so that it runs before `@emotion/core`
import React from 'react'
import createReactContext from 'create-react-context'

React.createContext = createReactContext

@stale
Copy link

stale bot commented Jul 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants