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

feat(server): add extractCriticalToChunks #2334

Merged
merged 36 commits into from
May 7, 2021
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
d98b963
init
mnajdova Apr 6, 2021
f30982d
add some resets
mnajdova Apr 6, 2021
bba6038
updates
mnajdova Apr 9, 2021
55b8c0e
fixes
mnajdova Apr 9, 2021
aca34c9
cleanup
mnajdova Apr 9, 2021
afdcbe5
Update packages/server/types/create-instance.d.ts
mnajdova Apr 11, 2021
52521b4
Update packages/server/src/create-instance/extract-critical2.js
mnajdova Apr 11, 2021
c7ac36c
adress comments
mnajdova Apr 12, 2021
d7e7f8c
tests
mnajdova Apr 12, 2021
c39f42a
flow fixes
mnajdova Apr 12, 2021
ee704e1
Improve auto-rehydration for global styles when using extractCritical2
Andarist Apr 13, 2021
104d539
Fixed constructStyleTags to include all ids correctly
Andarist Apr 13, 2021
6b5489e
Merge branch 'main' into feat/extract-critical-2
mnajdova Apr 14, 2021
cdd74fd
Update packages/server/types/create-instance.d.ts
mnajdova Apr 14, 2021
d5dad23
Update packages/server/src/create-instance/construct-style-tags.js
mnajdova Apr 14, 2021
de5acc6
fix flow issues & update tests
mnajdova Apr 14, 2021
da4d7d9
Update packages/react/__tests__/rehydration.js
mnajdova Apr 19, 2021
f3b63f4
tweak test title
Andarist Apr 18, 2021
8345672
Merge branch 'main' into feat/extract-critical-2
mnajdova Apr 19, 2021
e69351c
renamed to extractCriticalToChunks
mnajdova Apr 19, 2021
70301b5
spacings
mnajdova Apr 19, 2021
584216a
spacing
mnajdova Apr 19, 2021
00ce87b
Fixed an issue with Global styles being reinjected when rehydrating
Andarist Apr 22, 2021
124cf3f
resolve comments
mnajdova Apr 22, 2021
88de62d
test updates
mnajdova Apr 22, 2021
26b6254
Fixed issue with global styles being sometimes owned by multiple shee…
Andarist Apr 26, 2021
c550bc1
Add tests for global rehydration
Andarist Apr 26, 2021
9596d32
add comment about why the data-attribute is reset in <Global/>
Andarist Apr 26, 2021
b51b8d1
fix flow errors
Andarist Apr 26, 2021
1ba5ab8
Revert merging layout effects in <Global/>
Andarist May 1, 2021
8d6fc69
Update packages/server/src/create-instance/extract-critical-to-chunks.js
emmatown May 3, 2021
634c863
Merge branch 'main' into feat/extract-critical-2
emmatown May 3, 2021
2025657
Merge branch 'main' into feat/extract-critical-2
Andarist May 5, 2021
eb70989
Merge branch 'main' into feat/extract-critical-2
emmatown May 7, 2021
9537994
Create tidy-feet-buy.md
emmatown May 7, 2021
2cc16da
Create chilly-clocks-jam.md
emmatown May 7, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions docs/ssr.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,46 @@ import { renderToString } from 'react-dom/server'
import createEmotionServer from '@emotion/server/create-instance'
import createCache from '@emotion/cache'

const key = 'custom'
const cache = createCache({ key })
const { extractCritical2, constructStyleTags } = createEmotionServer(cache)

let element = (
<CacheProvider value={cache}>
<App />
</CacheProvider>
)

let { html, styles } = extractCritical2(renderToString(element))

res
.status(200)
.header('Content-Type', 'text/html')
.send(`<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
<title>My site</title>
${constructStyleTags({ html, styles })}
</head>
<body>
<div id="root">${html}</div>

<script src="./bundle.js"></script>
</body>
</html>`);
```

#### Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

For now, this won't get deprecated - the recommendation would be that:

  • extractCriticalToChunks should be used when using @emotion/react
  • extractCritical should be used when using @emotion/css

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, you can re-check the wording.


```jsx
import { CacheProvider } from '@emotion/react'
import { renderToString } from 'react-dom/server'
import createEmotionServer from '@emotion/server/create-instance'
import createCache from '@emotion/cache'

const key = 'custom'
const cache = createCache({ key })
const { extractCritical } = createEmotionServer(cache)
Expand Down
160 changes: 160 additions & 0 deletions packages/react/__tests__/rehydration.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,54 @@ afterEach(() => {
jest.clearAllMocks()
})

let React
let ReactDOM
let ReactDOMServer
let createCache
let css
let jsx
let CacheProvider
let Global
let createEmotionServer

const resetAllModules = () => {
jest.resetModules()

createCache = require('@emotion/cache').default
React = require('react')
ReactDOM = require('react-dom')
ReactDOMServer = require('react-dom/server')

const emotionReact = require('@emotion/react')
css = emotionReact.css
jsx = emotionReact.jsx
CacheProvider = emotionReact.CacheProvider
Global = emotionReact.Global
createEmotionServer = require('@emotion/server/create-instance').default
}

const removeGlobalProp = prop => {
let descriptor = Object.getOwnPropertyDescriptor(global, prop)
Object.defineProperty(global, prop, {
value: undefined,
writable: true,
configurable: true
})
// $FlowFixMe
return () => Object.defineProperty(global, prop, descriptor)
}

const disableBrowserEnvTemporarily = fn => {
let restoreDocument = removeGlobalProp('document')
let restoreWindow = removeGlobalProp('window')
let restoreHTMLElement = removeGlobalProp('HTMLElement')
try {
return fn()
} finally {
restoreDocument()
restoreWindow()
restoreHTMLElement()
}
}

test("cache created in render doesn't cause a hydration mismatch", () => {
Expand Down Expand Up @@ -166,3 +198,131 @@ test('initializing another Emotion instance should not move already moved styles
</head>
`)
})

test('xxx', () => {
const { app, styles } = disableBrowserEnvTemporarily(() => {
resetAllModules()

let cache = createCache({ key: 'mui' })
let { extractCritical2, constructStyleTags } = createEmotionServer(cache)

const rendered = ReactDOMServer.renderToString(
<CacheProvider value={cache}>
<Global styles={{ body: { color: 'white' } }} />
<Global styles={{ html: { background: 'red' } }} />
<main css={{ color: 'green' }}>
<div css={{ color: 'hotpink' }} />
</main>
</CacheProvider>
)
const extracted = extractCritical2(rendered)
return {
app: extracted.html,
styles: constructStyleTags(extracted)
}
})

safeQuerySelector('head').innerHTML = styles
safeQuerySelector('body').innerHTML = `<div id="root">${app}</div>`
expect(safeQuerySelector('html')).toMatchInlineSnapshot(`
<html>
<head>
<style
data-emotion="mui-global l6h"
>
body{color:white;}
</style>
<style
data-emotion="mui-global 10q49a4"
>
html{background:red;}
</style>
<style
data-emotion="mui bjcoli 1lrxbo5"
>
.mui-bjcoli{color:green;}.mui-1lrxbo5{color:hotpink;}
</style>
</head>
<body>
<div
id="root"
>
<main
class="mui-bjcoli"
>
<div
class="mui-1lrxbo5"
/>
</main>
</div>
</body>
</html>
`)

resetAllModules()
const cache = createCache({ key: 'mui' })

ReactDOM.render(
<CacheProvider value={cache}>
<Global styles={{ body: { color: 'white' } }} />
<Global styles={{ html: { background: 'red' } }} />
<main css={{ color: 'green' }}>
<div css={{ color: 'hotpink' }} />
</main>
</CacheProvider>,
safeQuerySelector('#root')
)

expect(safeQuerySelector('head')).toMatchInlineSnapshot(`
<head>
<style
data-emotion="mui-global"
data-s=""
>

body{color:white;}
</style>
<style
data-emotion="mui-global"
data-s=""
>

html{background:red;}
</style>
Copy link
Member

Choose a reason for hiding this comment

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

SSRed global style elements were replaced here by client-rendered ones, this all happens very quickly so this might not be noticeable (although I'm slightly worried about unmounting @font-face and stuff like that, even for a brief moment) but I would still try to check what could be done about it

Why does this happen? SSR style is immediately flushed here:

if (sheet.tags.length) {
// if this doesn't exist then it will be null so the style element will be appended
let element = sheet.tags[sheet.tags.length - 1].nextElementSibling
sheet.before = ((element: any): Element | null)
sheet.flush()
}

since we already have some rehydrated sheet.tags coming from here:
if (node !== null) {
sheet.hydrate([node])
}

I believe that the original intention was to flush this on updates but right now we are also flushing it when rehydrating 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this may be the reason for the flickering here - https://deploy-preview-25690--material-ui.netlify.app/ The text is black for a split second, then it changes to white, which is coming from the global styles. Although on this example, I am just omitting the global styles coming from extractCritical2, so it may be a different issue. Will have to double-check.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I can only see the color:white declaration in the SSRed HTML 🤔 Has this site been updated since then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot spot the flickering too this one time.. I’ve updated the example, we should see the borders after 5s too as that Global component stays mounted. Only the color should change to black (default)

Copy link
Member

Choose a reason for hiding this comment

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

@mitchellhamilton I would like to fix this issue with flushing and reinjecting Global styles when rehydrating. The easiest fix for that would be to just merge those 2 layout effects. Any objections to that? The first effect just avoids recreating StyleSheet (from what I can tell) based on the serialized.name but since sheet.flush is called anyway when it changes and highly dynamic global styles are rather rare I don't think we must maintain that split between two effects 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

Copy link
Member

Choose a reason for hiding this comment

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

I've implemented this change - could you take a look @mitchellhamilton ? I doubt that this can break anything but better to recheck this

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, reading the code, it seems as though the change doesn't preserve the behavior of inserting the new style tag in the same position as the previous one. Could you add a test based on https://codesandbox.io/s/still-leaf-invs2?file=/src/App.js?

<style
data-emotion="mui bjcoli 1lrxbo5"
data-s=""
>
.mui-bjcoli{color:green;}.mui-1lrxbo5{color:hotpink;}
</style>
</head>
`)

ReactDOM.render(
<CacheProvider value={cache}>
<Global styles={{ html: { background: 'red' } }} />
<main css={{ color: 'green' }}>
<div css={{ color: 'hotpink' }} />
</main>
</CacheProvider>,
safeQuerySelector('#root')
)

expect(safeQuerySelector('head')).toMatchInlineSnapshot(`
<head>
<style
data-emotion="mui-global"
data-s=""
>

html{background:red;}
</style>
<style
data-emotion="mui bjcoli 1lrxbo5"
data-s=""
>
.mui-bjcoli{color:green;}.mui-1lrxbo5{color:hotpink;}
</style>
</head>
`)
})
26 changes: 26 additions & 0 deletions packages/server/src/create-instance/construct-style-tags.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// @flow
import type { EmotionCache } from '@emotion/utils'
import { generateStyleTag } from './utils'

const createConstructStyleTags = (
cache: EmotionCache,
nonceString: string
) => (criticalData: {
html: string,
styles: Array<{ key: string, ids: Array<string>, css: string }>
}) => {
let styleTagsString = ''

criticalData.styles.forEach(item => {
styleTagsString += generateStyleTag(
item.key,
item.ids.join(' '),
item.css,
nonceString
)
})

return styleTagsString
}

export default createConstructStyleTags
51 changes: 51 additions & 0 deletions packages/server/src/create-instance/extract-critical2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// @flow
import type { EmotionCache } from '@emotion/utils'

const createExtractCritical2 = (cache: EmotionCache) => (html: string) => {
// parse out ids from html
// reconstruct css/rules/cache to pass
let RGX = new RegExp(`${cache.key}-([a-zA-Z0-9-_]+)`, 'gm')

let o = { html, styles: [] }
let match
let ids = {}
while ((match = RGX.exec(html)) !== null) {
// $FlowFixMe
if (ids[match[1]] === undefined) {
// $FlowFixMe
ids[match[1]] = true
}
}

const regularCssIds = []
let regularCss = ''

Object.keys(cache.inserted).forEach(id => {
if (
(ids[id] !== undefined ||
cache.registered[`${cache.key}-${id}`] === undefined) &&
cache.inserted[id] !== true
) {
if (cache.registered[`${cache.key}-${id}`]) {
// regular css can be added in one style tag
regularCssIds.push(id)
// $FlowFixMe
regularCss += cache.inserted[id].toString()
} else {
// each global styles require a new entry so it can be independently flushed
o.styles.push({
key: `${cache.key}-global`,
ids: [id],
css: cache.inserted[id]
})
}
}
})

// make sure that regular css is added after the global styles
o.styles.push({ key: cache.key, ids: regularCssIds, css: regularCss })

return o
}

export default createExtractCritical2
7 changes: 5 additions & 2 deletions packages/server/src/create-instance/index.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
// @flow
import type { EmotionCache } from '@emotion/utils'
import createExtractCritical from './extract-critical'
import createExtractCritical2 from './extract-critical2'
import createRenderStylesToString from './inline'
import createRenderStylesToStream from './stream'

import createConstructStyleTags from './construct-style-tags'
export default function(cache: EmotionCache) {
if (cache.compat !== true) {
// is this good? should we do this automatically?
Expand All @@ -13,7 +14,9 @@ export default function(cache: EmotionCache) {
const nonceString = cache.nonce !== undefined ? ` nonce="${cache.nonce}"` : ''
return {
extractCritical: createExtractCritical(cache),
extractCritical2: createExtractCritical2(cache),
renderStylesToString: createRenderStylesToString(cache, nonceString),
renderStylesToNodeStream: createRenderStylesToStream(cache, nonceString)
renderStylesToNodeStream: createRenderStylesToStream(cache, nonceString),
constructStyleTags: createConstructStyleTags(cache, nonceString)
}
}
26 changes: 13 additions & 13 deletions packages/server/src/create-instance/inline.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,6 @@
// @flow
import type { EmotionCache } from '@emotion/utils'

function generateStyleTag(
cssKey: string,
ids: string,
styles: string,
nonceString: string
) {
return `<style data-emotion="${cssKey} ${ids.substring(
1
)}"${nonceString}>${styles}</style>`
}
import { generateStyleTag } from './utils'

const createRenderStylesToString = (
cache: EmotionCache,
Expand Down Expand Up @@ -38,7 +28,12 @@ const createRenderStylesToString = (
}

if (globalStyles !== '') {
result = generateStyleTag(cssKey, globalIds, globalStyles, nonceString)
result = generateStyleTag(
cssKey,
globalIds.substring(1),
globalStyles,
nonceString
)
}

let ids = ''
Expand All @@ -50,7 +45,12 @@ const createRenderStylesToString = (
// $FlowFixMe
if (match[0] === '<') {
if (ids !== '') {
result += generateStyleTag(cssKey, ids, styles, nonceString)
result += generateStyleTag(
cssKey,
ids.substring(1),
styles,
nonceString
)
ids = ''
styles = ''
}
Expand Down
10 changes: 10 additions & 0 deletions packages/server/src/create-instance/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// @flow

export function generateStyleTag(
cssKey: string,
ids: string,
styles: string,
nonceString: string
) {
return `<style data-emotion="${cssKey} ${ids}"${nonceString}>${styles}</style>`
}
Loading