Fixed issue with cache created in render crashing SSRed site #1572

Andarist · 2019-10-25T21:27:18Z

This is currently just a proof-of-concept. I'd like to hear out the review before proceeding with this further.

Code like this:

const MyWrapper = ({ children, key = "stl" }) => {
  const myCache = createCache({ key })
  // I also have a custom container setup and some global styles but this is enough to reproduce the issue
  return <CacheProvider value={myCache}>{children}</CacheProvider>
}

is crashing SSRed site at runtime because React gets confused a lot by this as most likely it had already chance to recognize style tags as being rendered, but we move those styles inside of render (so during rehydration).

This is not a backward-compatible solution yet, because I had to change the rendered data attribute and the change affects more than one package so it's not self-contained. Thoughts on that? We can try to handle both cases at runtime (old and new one)

A little bit more detailed explanation

The issue was caused by server/client mismatch - React has started rendering (and thus seen the current state of the DOM with style tags rendered besides regular components). Moving style tags to a final container (document.head by default) happen inside createCache, so it was too late in the case of a cache being created in the render. At least that was what I've concluded from all of this.

I've tried to recreate repro case on CodeSandbox but I have no luck - all I can get is a mismatch error: https://codesandbox.io/s/jolly-jang-idu3r . I've also tried using production react-dom there but it didn't change anything.

I'm not entirely sure how React treats mismatched content but it seems like it tries to patch things up in this scenario - and as it has seen style as part of the rendered output (and that it's actually first child) it tries to:

add a copy of the "regular" child (a div with "Hello world" in this example), so if we pause with a debugger when React commit its mutation effects we can see for that moment that there are 2 the same children visible
removes the "seen" style tag - and it crashes on this step as it has already been moved by a side effect in createCache and the style tag has changed a parent (surprisingly for React)

changeset-bot · 2019-10-25T21:27:21Z

🦋 Changeset is good to go

Latest commit: b9b9a44

We got this.

This PR includes changesets to release 9 packages

Name	Type
@emotion/react	Major
@emotion/server	Major
@emotion/styled	Major
@emotion/cache	Major
@emotion/css	Major
@emotion/jest	Major
@emotion/primitives-core	Major
@emotion/native	Major
@emotion/primitives	Major

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

Andarist · 2019-10-25T21:27:59Z

packages/cache/src/index.js

+if (isBrowser) {
+  ssrStyles = document.querySelectorAll(`style[data-emotion]`)
+  Array.prototype.forEach.call(ssrStyles, (node: HTMLStyleElement) => {
+    document.head.appendChild(node)


we need to move those before render, head seems like a "safe place"

Andarist · 2019-10-25T21:28:39Z

packages/cache/src/index.js

-      // $FlowFixMe
-      attrib.split(' ').forEach(id => {
-        inserted[id] = true
-      })
      if (node.parentNode !== container) {


we might need to reparent here what already got moved previously to head if the container is a custom one

emmatown

This sounds look a good approach to me

Andarist · 2019-10-26T08:20:35Z

Ok, I'll try later adding backward compatibility and some tests. Thanks for the feedback.

emmatown · 2019-10-26T08:37:53Z

I think we might want to wait for v11 to do this because otherwise we have to decide between elements having different attributes which would be bad for users expecting them to have the attributes they have right now or writing both attributes which would mean larger SSR payloads.

Andarist · 2019-10-26T12:11:31Z

Yeah, you are probably right. I've created v11 milestone and added this and a bunch of other stuff to it. The list is certainly not complete yet, I need to go through all issues first before I call it complete - but if you have anything you'd like to add there, please feel free to do so 😉

JakeGinnivan · 2019-10-30T10:12:17Z

I am not sure this is a good idea, it makes emotion modules not pure because they effect things outside of themselves. This will affect tree shaking etc. I don't know how the automatic hydration works now though, so this may already be an issue.

Also wouldn't the example cause issues due to re-creating the cache on each render. If you wanted to support this scenario in addition to below you need to memoize createCache based on the key.

I have a few ideas on how this should be resolved

1. Introduce a factory to create MyWrapper

This doesn't affect emotion at all.

const createMyWrapper = key => {
  const myCache = createCache({ key })
  const MyWrapper = ({ children }) => {
    return <CacheProvider value={myCache}>{children}</CacheProvider>
  }
  return MyWrapper
}

const MyWrapper = createMyWrapper('stl')

export default () => (
  <MyWrapper>
    <Component />
  </MyWrapper>
)
function Component() {
  return <div css={css`
        color: green;
      `}>
    Hello world!
  </div>;
}

2. Update the hydrate function in create-emotion to do the hoisting to head

This also requires you rendering the <style data-emotion={key} />. It currently gets created as a side effect in insert => createStyleElement

Best place for this seems to be inside jsx.js, you could render an additional style element then mark it as inserted on the cache?

Then hydrate could look something like this:

function hydrate() {
  if (typeof document === 'undefined') {
    return
  }
  const prefixNodes = document.querySelectorAll('style[data-emotion]')
  Array.prototype.map.call(prefixNodes, prefixNode => prefixNode.getAttribute(`data-emotion`))
    .forEach(prefix => {
      const nodes = document.querySelectorAll(`style[data-emotion-${prefix}]`)
      Array.prototype.forEach.call(nodes, node => {
        document.head.appendChild(node)
      })
    })
}

JakeGinnivan · 2019-10-30T10:17:17Z

Example code for option 2

const MyWrapper = ({ children, key = "stl" }) => {
  const myCache = React.useRef(() => createCache({ key }))

  return <Fragment>
    <style data-emotion={key} />
    <CacheProvider value={myCache.current}>{children}</CacheProvider>
  </Fragment>
}

function hydrate() {
  if (typeof document === 'undefined') {
    return
  }
  const prefixNodes = document.querySelectorAll('style[data-emotion]')
  Array.prototype.map.call(prefixNodes, prefixNode => prefixNode.getAttribute(`data-emotion`))
    .forEach(prefix => {
      const nodes = document.querySelectorAll(`style[data-emotion-${prefix}]`)
      Array.prototype.forEach.call(nodes, node => {
        document.head.appendChild(node)
      })
    })
}

hydrate()
export default () => (
  <MyWrapper>
    <Component />
  </MyWrapper>
)
function Component() {
  return <div css={css`
        color: green;
      `}>
    Hello world!
  </div>;
}

Andarist · 2019-10-31T09:47:22Z

I am not sure this is a good idea, it makes emotion modules not pure because they effect things outside of themselves.

This definitely is a downside of this.

This will affect tree shaking etc.

Not that much, the side effect is short and concise - but yeah, in general this eager call cannot be tree shaken.

I don't know how the automatic hydration works now though, so this may already be an issue.

Automatic hydration happens at the moment inside of the createCache call - here:

emotion/packages/cache/src/index.js

Lines 68 to 83 in ed0a460

    
           if (isBrowser) { 
        
             container = options.container || document.head 
        
             const nodes = document.querySelectorAll(`style[data-emotion-${key}]`) 
        
             Array.prototype.forEach.call(nodes, (node: HTMLStyleElement) => { 
        
               const attrib = node.getAttribute(`data-emotion-${key}`) 
        
               // $FlowFixMe 
        
               attrib.split(' ').forEach(id => { 
        
                 inserted[id] = true 
        
               }) 
        
               if (node.parentNode !== container) { 
        
                 container.appendChild(node) 
        
               } 
        
             }) 
        
           }

For this to work automatically each request has to have its own unique cache because we need to track which exact rules are needed for the current SSR output and we want to avoid duplication when the same rule gets used multiple times. This cache gets created~ automatically here (it's created by BasicProvider):

emotion/packages/core/src/context.js

Lines 58 to 72 in ed0a460

    
           <EmotionCacheContext.Consumer> 
        
             {context => { 
        
               if (context === null) { 
        
                 return ( 
        
                   <BasicProvider> 
        
                     {newContext => { 
        
                       return func(props, newContext) 
        
                     }} 
        
                   </BasicProvider> 
        
                 ) 
        
               } else { 
        
                 return func(props, context) 
        
               } 
        
             }} 
        
           </EmotionCacheContext.Consumer>

Also wouldn't the example cause issues due to re-creating the cache on each render.

So as described above - this already happens (on the server-side with automatic SSR).

JakeGinnivan · 2019-10-31T12:42:00Z

Also wouldn't the example cause issues due to re-creating the cache on each render.

Understand on the server, it's only a single pass. But what about on the client? If the user is calling createCache inside a render method, I can't see any caching of the return value anywhere?

Andarist · 2019-11-02T22:38:13Z

Understand on the server, it's only a single pass. But what about on the client? If the user is calling createCache inside a render method, I can't see any caching of the return value anywhere?

The client is using different strategy - it uses plain default cache from context's default value and doesn't recreate it on each render (when not customizing cache etc).

JakeGinnivan · 2019-11-04T12:51:48Z

Yeah, but the issue was creating a custom cache with a different key during render?

emmatown · 2019-11-07T21:55:35Z

A consumer using a custom cache on the client should cache it themselves with useMemo

JakeGinnivan · 2019-11-08T00:57:29Z

Not sure where we are at.

But IMO I don't think this change is needed. The pattern used in the original issue is broken for a few reasons.

I think you should just add some docs that caches should not be created during rendering if using SSR. There are heaps of ways to solve the issue once you put that constraint on.

emmatown · 2019-11-08T03:54:26Z

Why is the pattern in the original issue broken besides the fact that the creation of the cache wasn’t memoized?

It’s also important to call out that we need to move some style tags on initial load always anyway because of the default cache so this doesn’t really cause any more code to run

JakeGinnivan · 2019-11-08T04:00:01Z

Why is the pattern in the original issue broken besides the fact that the creation of the cache wasn’t memoized?

Memoizing the cache creation still causes the original issue because style tags are being removed during a render.

The default cache works because this is done when you load the library, not during initial hydration.

I don't mind this change, as long as it doesn't impact extract critical

emmatown · 2019-11-08T04:07:20Z

This PR moves all the tags to the head before render so that’s not an issue since at most, the styles tags will be moved from the head to somewhere else and importantly they won’t be in the tree managed by React so it won’t cause any problems.(though in most cases they’ll stay in the head)

JakeGinnivan · 2019-11-08T04:09:58Z

The docs would just need to include memoizing the cache (or building this in), and instructions on how to hydrate if that isn't done automatically.

Andarist · 2019-11-14T21:21:46Z

I think we all here understand the core issue (based on recent comments) - but some older comments look a little bit confused, so I've included a little bit more thorough explanation in the original post.

After some thinking about this, I'm more convinced now that we shouldn't merge this. This requires some side-effectful code being executed in a top-level scope during initialization and I would consider this to be an anti-pattern. This can never be eliminated by minification tools - which is not that big of a deal but indicates a code smell. The whole thing of moving style tags around is already a hack (a clever one) and I don't think we need to make it even hackier. Its purpose is to provide a 0 config SSR, but when one creates a custom cache it already is past the "0 config" point. It would, of course, be better if things would just work under all possible circumstances - but in here we really have received a single report about this in 1 year in which emotion@10 is out, so it seems like documentation around this can be enough.

We could try to think how to warn people at runtime about this better - one thing that comes to my mind that we could track in dev jsx calls (so basically rendering) and createCache calls to determine if createCache has been called during render or before it.

Other than that we could also propose a solution for this as it seems like a use case itself is valid. It might in some edge case scenarios be easier to create cache during render (where one has access to props etc) rather than per request (although it's always possible to create it per request). A solution would be to just move those style tags "manually" to head or any other destination container of choosing (we could make it easier by exposing a helper for this - which we would use ourselves internally as well)

emmatown · 2019-11-14T21:27:54Z

We already have to move style elements for the default cache to the head though so this only means that we have to move some more style elements so I don't really see the problem?

Andarist · 2019-11-14T22:05:10Z

What we do currently is a little bit hacky (although clever) - but it's at least contained within a call that returns something. This has potential (mostly theoretical) of being removed by DCE.

What I don't like the most after sitting on this PR for a bit that it's guesswork - we try to move everything into the head and later we try to correct ourselves when this is not what was supposed to happen. The current solution is at least always targeted from the start.

Andarist · 2019-12-02T07:01:50Z

@mitchellhamilton any more thoughts on this one? How do we want to proceed?

emmatown · 2019-12-02T23:38:01Z

I'm still of the opinion that we should do this.

What we do currently is a little bit hacky (although clever) - but it's at least contained within a call that returns something. This has potential (mostly theoretical) of being removed by DCE.

I don't think this really matters since we will always create a default cache so we're always going to be running this code

What I don't like the most after sitting on this PR for a bit that it's guesswork - we try to move everything into the head and later we try to correct ourselves when this is not what was supposed to happen. The current solution is at least always targeted from the start.

While, yes, it will sometimes be wrong, I think it's good enough because in most cases the container option is used for iframes rather than a custom container in the same page and given that the SSRed style tags aren't in the container before they're moved, I don't think it's a big deal.

Andarist · 2019-12-08T11:14:13Z

I don't think this really matters since we will always create a default cache so we're always going to be running this code

The problem I see is about transitive dependencies. Imagine a situation like this - a component library built without emotion but with its select component built on top of react-select which in turn depends on @emotion/core. And a consumer of that component library importing just a Button - ideally in this situation no react-select & @emotion/core code should land in the final bundle. Right now top-level side effects of @emotion/core will have to land in it though and this particular thing can't be optimized away with #__PURE__. Technically, maybe, sideEffects: false in our package.json could help with that but I don't trust this particular hint - its semantics are not set in stone nor described comprehensively anywhere and it's easy for some tool to implement it differently than existing tools which could break us.

Maybe we could wrap this side effect inside a createDefaultContext which could be annotated with #__PURE__ and would only be retained in the bundle if emotion gets used. I still rather believe it would be better to split those responsibilities though (and require a user to call an extra function upfront) - it's a little bit too magical for me that a "create*" method would perform such a side effect.

Andarist · 2019-12-22T22:36:18Z

@mitchellhamilton I've roughly adjusted the code. Please review with extra care as this might be considered a major breaking change - it changes all data-emotion~ attributes.

emmatown · 2019-12-23T05:18:57Z

packages/core/src/context.js

+    // document.head is a safe place to move them to
+    Array.prototype.forEach.call(ssrStyles, (node: HTMLStyleElement) => {
+      ;((document.head: any): HTMLHeadElement).appendChild(node)
+    })


I think this should happen inside @emotion/cache since otherwise this will be broken for emotion/to be renamed to @emotion/css

How could it affect vanilla emotion? This thing here only fixes moving "inline" style tags (produced by zero-config SSR) - vanilla emotion is not affected by this as it doesn't produce such style tags, so it should be safe to have this in @emotion/core

You can use renderStylesToString and renderStylesToNodeStream with vanilla emotion which both produce inline style tags

emmatown · 2019-12-23T05:45:42Z

packages/core/__tests__/__snapshots__/global.js.snap

  >

-    h1{-webkit-animation:animation-ocj8pk 1s;animation:animation-ocj8pk 1s;}
+    @keyframes animation-ocj8pk{from,to{color:green;}50%{color:hotpink;}}


This reordering of things worries me, could we revert the global refactoring since from what I understand, it's solving a bug or anything, only size/code niceness, right?

This change could have fixed a slight bug with sheet.before - but I need to give it slightly more thought. Probably better to move this out of this PR so I've reverted this change for now.

codecov · 2019-12-23T10:55:36Z

Codecov Report

Merging #1572 into next will decrease coverage by 0.02%.
The diff coverage is 92.85%.

Impacted Files	Coverage Δ
packages/styled/src/base.js	`100% <ø> (ø)`	⬆️
packages/react/src/jsx.js	`100% <ø> (ø)`	⬆️
packages/react/src/class-names.js	`100% <ø> (ø)`	⬆️
packages/server/src/create-instance/stream.js	`100% <ø> (ø)`	⬆️
packages/react/src/context.js	`100% <100%> (ø)`	⬆️
packages/server/src/create-instance/inline.js	`100% <100%> (ø)`	⬆️
packages/server/test/util.js	`94.44% <100%> (+0.44%)`	⬆️
packages/css/src/index.js	`100% <100%> (ø)`	⬆️
packages/react/src/global.js	`97.61% <66.66%> (+0.05%)`	⬆️
packages/cache/src/index.js	`98% <94.11%> (-0.92%)`	⬇️

Andarist · 2019-12-26T10:00:44Z

After some more thinking I have one more concern about this (although it's not strictly exclusive to this change, it relates to it).

Because of one of the latest changes - https://github.com/emotion-js/emotion/pull/1696/files#diff-af18f1f9e18306ed3fc3d28811507a87R5 - cache/sheet owns its SSRed styles, which is a good thing because they are just another styles, only inserted to the document in the other point in time (SSR). Given this fact, we shouldn't allow for a situation where a single style tag is owned by multiple sheets/caches and right now it happens by default - because the same key (css) is used for 2 default caches (one in emotion, one in @emotion/core). We cannot escape associating keys with sheets/caches, CSS is global and such - but we should at least try to warn users when 2 caches are created with the same key.

I see 2 options how we could handle this:

those 2 default caches should have different keys
there should be a single default cache with css key, used by both libraries - but this would force us to implement couple of stuff differently (like .compat handling etc)

emmatown · 2019-12-31T07:35:16Z

I'm sure I really understand the problem with style tags being "owned" by multiple sheets/what problems it would cause?

Andarist · 2019-12-31T07:43:03Z

Flushing is the problem. One cache might call flush and it might remove styles inserted by a different one.

EDIT:// oh - and even worse problem than flushing. Different caches can have different containers, we cant allow for a newly created cache to “hijack” existing styles into its custom container

emmatown · 2020-01-15T23:35:06Z

I think that those problems are acceptable since flushing is a pretty rare thing and hijacking styles into a custom container shouldn't be a problem since you'll only use a cache with a custom container in one of two scenarios:

an iframe which means you're not server rendering and it's a different document
a custom cache in the same page with different stylis plugins, or etc in which case you should be using a custom key

emmatown · 2020-01-15T23:35:28Z

Idea: what if we made key a required option on the cache

Andarist · 2020-01-16T23:01:42Z

Yes, this is pretty much my thinking - with the addition of introducing dev-only warnings about duplicate keys (maybe just for browser envs as on the server caches are recreated each time). That would also kinda imply that maybe we should use different keys for @emotion/react and @emotion/css.

Andarist · 2020-03-14T19:58:05Z

@mitchellhamilton Ok, I've rebased and reimplemented this slightly. Would appreciate a re-review.

Andarist · 2020-03-15T09:55:05Z

@mitchellhamilton good catch with that options argument! anything else we'd like to change here? or is this ready to merge?

packages/cache/src/index.js

emmatown

Needs a changeset but other than that, LGTM

Andarist · 2020-03-16T11:19:59Z

I've added changesets & I'm merging this in, but please take a look at them.

…-js#1572) * Fixed issue with cache created in render crashing SSRed site * Update index.d.ts * Update packages/cache/src/index.js * Fix flow errors * add changesets Co-authored-by: Mitchell Hamilton <mitchell@hamil.town>

Andarist commented Oct 25, 2019

View reviewed changes

Andarist requested a review from emmatown October 25, 2019 21:28

Andarist mentioned this pull request Oct 25, 2019

Issue when using CacheProvider in Gatsby (SSR) #1468

Closed

emmatown reviewed Oct 26, 2019

View reviewed changes

Andarist added this to the v11 milestone Oct 26, 2019

Andarist mentioned this pull request Oct 29, 2019

Using emotion with SSR and rehydration results in React error #1288

Closed

emmatown assigned Andarist Nov 7, 2019

Andarist requested a review from emmatown December 22, 2019 22:36

emmatown reviewed Dec 23, 2019

View reviewed changes

Andarist force-pushed the fix/cache-in-render-ssr-mismatch branch from e41f16f to 746e37a Compare December 23, 2019 10:55

Andarist force-pushed the fix/cache-in-render-ssr-mismatch branch from dc2ff9d to 3199270 Compare March 14, 2020 19:04

Fixed issue with cache created in render crashing SSRed site

7966846

Andarist force-pushed the fix/cache-in-render-ssr-mismatch branch from 3199270 to 7966846 Compare March 14, 2020 19:44

Andarist requested a review from emmatown March 14, 2020 19:58

Update index.d.ts

c4254b9

emmatown reviewed Mar 15, 2020

View reviewed changes

packages/cache/src/index.js Outdated Show resolved Hide resolved

Update packages/cache/src/index.js

5bce93a

emmatown approved these changes Mar 15, 2020

View reviewed changes

Andarist added 2 commits March 16, 2020 12:06

Fix flow errors

7b6f70c

add changesets

b9b9a44

Andarist merged commit 105de5c into next Mar 16, 2020

Andarist deleted the fix/cache-in-render-ssr-mismatch branch March 16, 2020 11:20

github-actions bot mentioned this pull request Mar 16, 2020

Version Packages (next) #1735

Merged

github-actions bot mentioned this pull request Nov 10, 2020

Version Packages #2046

Merged

Fixed issue with cache created in render crashing SSRed site #1572

Fixed issue with cache created in render crashing SSRed site #1572

Conversation

Andarist commented Oct 25, 2019 • edited Loading

changeset-bot bot commented Oct 25, 2019 • edited Loading

🦋 Changeset is good to go

Andarist Oct 25, 2019

Choose a reason for hiding this comment

Andarist Oct 25, 2019

Choose a reason for hiding this comment

emmatown left a comment

Choose a reason for hiding this comment

Andarist commented Oct 26, 2019

emmatown commented Oct 26, 2019

Andarist commented Oct 26, 2019

JakeGinnivan commented Oct 30, 2019

1. Introduce a factory to create MyWrapper

2. Update the hydrate function in create-emotion to do the hoisting to head

JakeGinnivan commented Oct 30, 2019 • edited Loading

Andarist commented Oct 31, 2019

JakeGinnivan commented Oct 31, 2019

Andarist commented Nov 2, 2019

JakeGinnivan commented Nov 4, 2019

emmatown commented Nov 7, 2019

JakeGinnivan commented Nov 8, 2019

emmatown commented Nov 8, 2019

JakeGinnivan commented Nov 8, 2019

emmatown commented Nov 8, 2019

JakeGinnivan commented Nov 8, 2019

Andarist commented Nov 14, 2019

emmatown commented Nov 14, 2019

Andarist commented Nov 14, 2019

Andarist commented Dec 2, 2019

emmatown commented Dec 2, 2019 • edited Loading

Andarist commented Dec 8, 2019

Andarist commented Dec 22, 2019

emmatown Dec 23, 2019

Choose a reason for hiding this comment

Andarist Dec 23, 2019

Choose a reason for hiding this comment

emmatown Dec 24, 2019

Choose a reason for hiding this comment

emmatown Dec 23, 2019

Choose a reason for hiding this comment

Andarist Dec 23, 2019

Choose a reason for hiding this comment

codecov bot commented Dec 23, 2019 • edited Loading

Codecov Report

Andarist commented Dec 26, 2019

emmatown commented Dec 31, 2019

Andarist commented Dec 31, 2019 • edited Loading

emmatown commented Jan 15, 2020

emmatown commented Jan 15, 2020

Andarist commented Jan 16, 2020

Andarist commented Mar 14, 2020

Andarist commented Mar 15, 2020

emmatown left a comment

Choose a reason for hiding this comment

Andarist commented Mar 16, 2020

Andarist commented Oct 25, 2019 •

edited

Loading

changeset-bot bot commented Oct 25, 2019 •

edited

Loading

JakeGinnivan commented Oct 30, 2019 •

edited

Loading

emmatown commented Dec 2, 2019 •

edited

Loading

codecov bot commented Dec 23, 2019 •

edited

Loading

Andarist commented Dec 31, 2019 •

edited

Loading