feat(server): add extractCriticalToChunks #2334

mnajdova · 2021-04-06T10:52:06Z

What:

This PR aims to fix #2158 and possibly #2049

Why:

The new implementation of extractCritical should allow developers to solve issues with SSR, mostly with the order of injecting the global css as well as fix dynamic re-writes of the global styles.

How:

It is implemented by adding a new extractCriticalToChunks utility that will return HTML and an array of styles objects each containing css string and an array of ids. Each injectGlobal or <Global /> component will be one item in the array, so that it can be treated independently. The css for the regular styles is added to one item of the list.

Checklist:

Documentation
Tests
Code complete
Changeset

changeset-bot · 2021-04-06T10:52:08Z

🦋 Changeset detected

Latest commit: 2cc16da

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@emotion/react	Minor
@emotion/server	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/server/src/create-instance/extract-critical2.js

docs/ssr.mdx

mnajdova · 2021-04-06T10:53:10Z

I am not sure if this is all that we need, to be honest. I think that one problem may be that the ids still contain the ids of the global styles too, not sure if this will be problematic.

codesandbox-ci · 2021-04-06T10:55:20Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2cc16da:

Sandbox	Source
Emotion	Configuration
trusting-elbakyan-4k964	Issue #2158

docs/ssr.mdx

packages/server/src/create-instance/extract-critical2.js

mnajdova · 2021-04-09T22:30:46Z

I may have a bit of a different proposal. As emotion is already automatically adding and removing the <style data-emotion="global-css" /> maybe we should just not include the global styles in the result of extractCritical. The general problem is that, when some of the <Global /> components will be removed, extractCritical is not invoked again, so the global styles are still present in the <head />. I've created mui/material-ui#25690 for easier testing, as everything is already set up. I've updated here the root of the docs to return one global styles that sets * { color: white } and I am removing it after 5s. In order to make this scenario work, I just needed to add the regular css from the list I am returning now with the new extractCritical. Adding all style tags separately doesn't help, as this method is not being invoked again, when the <Global /> is being removed, so the previously added <style /> tags for the global styles are still there, see https://github.com/mui-org/material-ui/pull/25690/files#diff-51bb0d154187975c21ed74b384db0dac87cc6ea7079dfa2e7402e0ad8f40a563R160-R163

Andarist

We'd need to a test like this:

SSR <Global/><Global/><div css={{}}/> using extractCritical2
rehydrate that
unmount one of <Global/>s

The expected outcome is that only those styles would be removed and that they would be removed successfully (which I might sometimes refer to as flushing).

I think that this might require adding key to the items returned by extractCritical2 as <Global/> styles have keys suffixed with *-global (from what I recall).

A second concern is that we should check how this is going to work when rehydrating with @emotion/css. It would be great if we could add the ability to unmount injectGlobal styles:

emotion/packages/css/src/create-instance.js

Line 96 in 4d7efcb

insertWithoutScoping(cache, serialized)

. However, right now @emotion/css assumes a single sheet so flush is easy and straightforward:

emotion/packages/css/src/create-instance.js

Lines 112 to 116 in 4d7efcb

    
           flush() { 
        
             cache.registered = {} 
        
             cache.inserted = {} 
        
             cache.sheet.flush() 
        
           },

The ability to unmount injectGlobal styles would require maintaining multiple sheets (this is what <Global/> kinda does) and this might be a little bit tricky/unclear for @emotion/css

@emotion/css stuff is not a must here but would be great if we could at least take a closer look at it to assess how problematic implementing this for it would be.

packages/server/types/create-instance.d.ts

packages/server/src/create-instance/extract-critical2.js

packages/server/src/create-instance/inline.js

packages/server/src/create-instance/extract-critical2.js

packages/server/test/util.js

packages/server/src/create-instance/construct-style-tags.js

packages/server/src/create-instance/extract-critical2.js

Co-authored-by: Mateusz Burzyński <[email protected]>

mnajdova · 2021-04-12T12:29:18Z

I'll need some help with the test.

We'd need to a test like this:

SSR
using extractCritical2

rehydrate that

unmount one of s

The expected outcome is that only those styles would be removed and that they would be removed successfully (which I might sometimes refer to as flushing).

I've tried implementing this with d7e7f8c (I've updated the components), but I am missing the step 2

rehydrate that

Will it be enough if I just call emotion.hydrate(ids)? Currently the result of the second re-rendering is identical with the first one, which is because of the input (cache) coming in extractCritical2.

Andarist · 2021-04-13T16:13:50Z

I've tried implementing this with d7e7f8c (I've updated the components), but I am missing the step 2

I've first done this but this wasn't quite "e2e" just yet so I've followed up with this. In the process I've fixed some small~ issues that were in there - so even if my test is somewhat complicated, it was still worth doing this. I've pushed those commits to your branch so please fetch them before you continue working on this :)

I'll leave some additional comments in a moment but those will just be about things I've noticed so far - gonna try to follow up with a full review in the following days.

packages/react/__tests__/rehydration.js

Andarist · 2021-04-13T16:21:01Z

packages/react/__tests__/rehydration.js

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


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:

emotion/packages/react/src/global.js

Lines 126 to 131 in 4d7efcb

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:

emotion/packages/react/src/global.js

Lines 108 to 110 in 4d7efcb

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 😕

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.

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

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)

@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 🤔

Sounds good

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

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?

packages/server/src/create-instance/construct-style-tags.js

packages/server/src/create-instance/inline.js

packages/server/types/create-instance.d.ts

packages/react/__tests__/rehydration.js

Co-authored-by: Mateusz Burzyński <[email protected]>

Andarist · 2021-04-22T10:55:04Z

packages/server/test/util.js

+  }
+}
+
+export const prettifyCritical2 = ({


as this critical2 was kinda renamed this should be adjusted as well

Andarist · 2021-04-22T10:57:51Z

packages/server/types/create-instance.d.ts

 export interface EmotionServer {
  extractCritical(html: string): EmotionCritical
  renderStylesToString(html: string): string
  renderStylesToNodeStream(): NodeJS.ReadWriteStream
+  constructStyleTags(html: string): EmotionCriticalToChunks


This is not correct. The return type of this is a string but the interface definition is missing a declaration for extractCriticalToChunks - for which this return type would be correct.

wdyt about renaming constructStyleTags to constructStyleTagsFromChunks? That way it would be more clear that those 2 APIs can be used together (extractCriticalToChunks and constructStyleTagsFromChunks)

mnajdova · 2021-04-22T13:51:13Z

@Andarist I've tried resolving last comments with 124cf3f, 88de62d

mnajdova · 2021-04-22T14:33:49Z

@Andarist I've upgraded our test branch on Material-UI, and I got the error below when navigating from one page to another. I think it's related to 00ce87b. It fails on sheet.flush(). I haven't seen the issue before that commit.

Unhandled Runtime Error
TypeError: Cannot read property 'removeChild' of null

Call Stack
<unknown>
node_modules\@emotion\sheet\dist\emotion-sheet.browser.esm.js (133:0)
Array.forEach
<anonymous>
StyleSheet.flush
/_next/static/chunks/pages/_app.js (2676:15)
<unknown>
node_modules\@emotion\react\dist\emotion-react.browser.esm.js (158:0)
HTMLUnknownElement.callCallback

Repro steps:

Visit: https://deploy-preview-25690--material-ui.netlify.app/branding/about/
Click the Docs link on the top of the page
Error is thrown

We use different <Global /> components on these two pages.

…ts after hydration

Andarist · 2021-04-26T06:19:23Z

@mnajdova I believe that I've fixed the issue & added tests for this - I've also confirmed that in your repository.

packages/react/src/global.js

Andarist · 2021-04-26T06:31:50Z

packages/react/__tests__/rehydration.js

+  `)
+})
+
+test('duplicated global styles can be removed safely after rehydrating HTML SSRed with zero config approach', () => {


I thought that this case could be failing so I've added a test for this but it turned out that it wasn't failing.

I was under the impression that this query would select the same node twice when looking for the same hash:

emotion/packages/react/src/global.js

Lines 98 to 100 in 9596d32

let node: HTMLStyleElement | null = document.querySelector(

`style[data-emotion="${key} ${serialized.name}"]`

)

but:

we SSR those style elements as [0, 1]

they are moved automatically to head when initializing, and the order is preserved: [0, 1]

when rehydrating the first global it grabs the element 0 and rehydrates it which moves that element before all other tags from the main cache so we end up temporarily with [1 , 0, ...mainCache] order

the second global thus now grabs the element 1 and also moves It before other tags from the main chance and we end up with [0, 1, ...mainCache]

I think this works OK and I couldn't find a situation in which it would break but maybe @mitchellhamilton would have some idea for an edge case here that could be tested.

mnajdova · 2021-04-26T11:27:32Z

@mnajdova I believe that I've fixed the issue & added tests for this - I've also confirmed that in your repository.

Great! I double-checked all failing scenarios we had, no issues found 🚀

Andarist · 2021-05-02T09:28:59Z

Ok, I'll be taking a last look at this in the following days but I think it's in a good shape and that there are no major blockers against merging this. If you, @mitchellhamilton, have some spare time I would also appreciate your review here since this is touching some core logic that we need to get right.

packages/server/src/create-instance/extract-critical-to-chunks.js

init

d98b963

mnajdova commented Apr 6, 2021

View reviewed changes

packages/server/src/create-instance/extract-critical2.js Outdated Show resolved Hide resolved

mnajdova commented Apr 6, 2021

View reviewed changes

docs/ssr.mdx Outdated Show resolved Hide resolved

add some resets

f30982d

Andarist reviewed Apr 8, 2021

View reviewed changes

docs/ssr.mdx Outdated Show resolved Hide resolved

packages/server/src/create-instance/extract-critical2.js Outdated Show resolved Hide resolved

mnajdova added 2 commits April 9, 2021 17:58

updates

bba6038

fixes

55b8c0e

mnajdova marked this pull request as ready for review April 9, 2021 21:43

mnajdova requested a review from Andarist April 9, 2021 22:14

cleanup

aca34c9

mnajdova mentioned this pull request Apr 9, 2021

[docs] Fix dynamic global styles mui/material-ui#25690

Merged

Andarist reviewed Apr 10, 2021

View reviewed changes

mnajdova and others added 4 commits April 11, 2021 07:27

Update packages/server/types/create-instance.d.ts

afdcbe5

Co-authored-by: Mateusz Burzyński <[email protected]>

Update packages/server/src/create-instance/extract-critical2.js

52521b4

Co-authored-by: Mateusz Burzyński <[email protected]>

adress comments

c7ac36c

tests

d7e7f8c

flow fixes

c39f42a

mnajdova requested a review from Andarist April 12, 2021 13:01

This comment has been minimized.

Sign in to view

Andarist added 2 commits April 13, 2021 18:02

Improve auto-rehydration for global styles when using extractCritical2

ee704e1

Fixed constructStyleTags to include all ids correctly

104d539

Andarist reviewed Apr 13, 2021

View reviewed changes

mnajdova and others added 2 commits April 14, 2021 11:49

Merge branch 'main' into feat/extract-critical-2

6b5489e

Update packages/server/types/create-instance.d.ts

cdd74fd

Co-authored-by: Mateusz Burzyński <[email protected]>

Andarist reviewed Apr 22, 2021

View reviewed changes

mnajdova added 2 commits April 22, 2021 15:15

resolve comments

124cf3f

test updates

88de62d

Andarist added 2 commits April 26, 2021 07:19

Fixed issue with global styles being sometimes owned by multiple shee…

26b6254

…ts after hydration

Add tests for global rehydration

c550bc1

add comment about why the data-attribute is reset in <Global/>

9596d32

Andarist reviewed Apr 26, 2021

View reviewed changes

packages/react/src/global.js Show resolved Hide resolved

Andarist reviewed Apr 26, 2021

View reviewed changes

fix flow errors

b51b8d1

mnajdova mentioned this pull request Apr 28, 2021

[Docs] Typography is dark text in dark mode mui/material-ui#26008

Closed

2 tasks

Revert merging layout effects in <Global/>

1ba5ab8

emmatown reviewed May 3, 2021

View reviewed changes

packages/server/src/create-instance/extract-critical-to-chunks.js Outdated Show resolved Hide resolved

Update packages/server/src/create-instance/extract-critical-to-chunks.js

8d6fc69

emmatown approved these changes May 3, 2021

View reviewed changes

emmatown and others added 2 commits May 3, 2021 11:00

Merge branch 'main' into feat/extract-critical-2

634c863

Merge branch 'main' into feat/extract-critical-2

2025657

Andarist approved these changes May 5, 2021

View reviewed changes

mnajdova mentioned this pull request May 6, 2021

[docs] Various text colors are not legible in dark mode mui/material-ui#26158

Closed

2 tasks

emmatown added 3 commits May 7, 2021 10:13

Merge branch 'main' into feat/extract-critical-2

eb70989

Create tidy-feet-buy.md

9537994

Create chilly-clocks-jam.md

2cc16da

emmatown merged commit 7d9e74f into emotion-js:main May 7, 2021

github-actions bot mentioned this pull request May 7, 2021

Version Packages #2365

Merged

Andarist mentioned this pull request Nov 9, 2021

fix(styled): Ensure no hydration mismatch with React.useId #2542

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): add extractCriticalToChunks #2334

feat(server): add extractCriticalToChunks #2334

mnajdova commented Apr 6, 2021 •

edited

Loading

changeset-bot bot commented Apr 6, 2021 •

edited

Loading

mnajdova commented Apr 6, 2021

codesandbox-ci bot commented Apr 6, 2021 •

edited

Loading

mnajdova commented Apr 9, 2021 •

edited

Loading

Andarist left a comment

mnajdova commented Apr 12, 2021

This comment has been minimized.

Andarist commented Apr 13, 2021

Andarist Apr 13, 2021

mnajdova Apr 14, 2021

Andarist Apr 18, 2021

mnajdova Apr 19, 2021

Andarist Apr 20, 2021

emmatown Apr 20, 2021

Andarist Apr 22, 2021

emmatown Apr 22, 2021

Andarist Apr 22, 2021

Andarist Apr 22, 2021

mnajdova commented Apr 22, 2021

mnajdova commented Apr 22, 2021

Andarist commented Apr 26, 2021

Andarist Apr 26, 2021

mnajdova commented Apr 26, 2021

Andarist commented May 2, 2021

	flush() {
	cache.registered = {}
	cache.inserted = {}
	cache.sheet.flush()
	},

	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()
	}

	let node: HTMLStyleElement \| null = document.querySelector(
	`style[data-emotion="${key} ${serialized.name}"]`
	)

feat(server): add extractCriticalToChunks #2334

feat(server): add extractCriticalToChunks #2334

Conversation

mnajdova commented Apr 6, 2021 • edited Loading

changeset-bot bot commented Apr 6, 2021 • edited Loading

🦋 Changeset detected

mnajdova commented Apr 6, 2021

codesandbox-ci bot commented Apr 6, 2021 • edited Loading

mnajdova commented Apr 9, 2021 • edited Loading

Andarist left a comment

Choose a reason for hiding this comment

mnajdova commented Apr 12, 2021

This comment has been minimized.

Andarist commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Apr 22, 2021

mnajdova commented Apr 22, 2021

Andarist commented Apr 26, 2021

Choose a reason for hiding this comment

mnajdova commented Apr 26, 2021

Andarist commented May 2, 2021

mnajdova commented Apr 6, 2021 •

edited

Loading

changeset-bot bot commented Apr 6, 2021 •

edited

Loading

codesandbox-ci bot commented Apr 6, 2021 •

edited

Loading

mnajdova commented Apr 9, 2021 •

edited

Loading