Fix JSXStyle renders styles too late

[giuseppeg]

We tried master on zeit.co and it seems that when we have the side effects in componentDidMount styles are rendered after the content causing transitions to apply to properties that wouldn't otherwise be animated e.g. padding similarly to this https://jsfiddle.net/uwbm165z/ (unfortunately I wasn't able to reproduce with React yet but here is my attempt https://codesandbox.io/s/p7q6n6r35m).

In this patch we move the side effect styleSheetRegistry.add to render making sure that shouldComponentUpdate doesn't trigger unnecessary re-renderings. On update we then remove the old styles early enough aka on getSnapshotBeforeUpdate.

@bvaughn this is a "followup" of the conversation we had in #457 (comment) Does it make sense?

[bvaughn]

Hm. This looks unsafe for the reasons mentioned here.

getSnapshotBeforeUpdate is meant to be read-only, but...I wonder if you considered trying it for the stylesheet modification? At least it would only be called during (right before) commit.

[bvaughn]

Can you explain what this means or what it's doing?

[giuseppeg]

On update we then remove the old styles early enough aka on getSnapshotBeforeUpdate.

[giuseppeg]

this is the case when you have dynamic styles i.e. interpolations (tmpl literals expressions) that are based on props

[giuseppeg]

Hm. This looks unsafe for the reasons mentioned here.

My guess is that because this component is a child of the elements it styles, the styles are rendered slightly late (ha ha).

Anyhow if sCU is solid this should render (as for render is called only once during the render phase) only once right?

getSnapshotBeforeUpdate is meant to be read-only, but...I wonder if you considered trying it for the stylesheet modification? At least it would only be called during (right before) commit.

I could do that too, would it be safe to keep a class properly to track the mounted state? I need to call styleSheetRegistry.remove only on updates (not on mounting). Also for ssr i’d need the constructor hack I guess

[bvaughn]

My guess is that because this component is a child of the elements it styles, the styles are rendered slightly late (ha ha).

Still doesn't seem right to me. Changes to the DOM are committed synchronously, so there should only be one layout+paint. 😕

Anyhow if sCU is solid this should render (as for render is called only once during the render phase) only once right?

No. Render can be called multiple times– if there's an error, or if your app is using the new async rendering mode.

I could do that too, would it be safe to keep a class properly to track the mounted state? I need to call styleSheetRegistry.remove only on updates (not on mounting).

Yes, assuming you only set that property in componentDidMount– that sounds safe.

Also for ssr i’d need the constructor hack I guess

For now ☹️ unless/until we land this new lifecycle.

[giuseppeg]

Still doesn't seem right to me. Changes to the DOM are committed synchronously, so there should only be one layout+paint.

Yeah I will try harder and see if I can manage to make a test case.

No. Render can be called multiple times– if there's an error, or if your app is using the new async rendering mode.
Yes, assuming you only set that property in componentDidMount– that sounds safe.

So it seems that for our case the safest thing to do is to have all the side effects in getSnapshotBeforeUpdate 👌

[giuseppeg]

@bvaughn what I don't understand is this part:

DOM are committed synchronously, so there should only be one layout+paint

Is everything committed after componentDidMount? If so how is it possible that I can get a ref to the DOM in didMount? To me it seems that the DOM is already there and we apply the styles with our side effect.

[bvaughn]

No. The DOM is mutated before componentDidMount is called– but componentDidMount is called synchronously (right afterward). The browser doesn't do layout/paint until React's work is done.

This is the reason that cascading updates (re-renders caused by e.g. componentDidMount or componentDidUpdate) aren't visible to users– React processes them and applies the changes synchronously.

[giuseppeg]

interesting, it is not quite clear to me how layout and paint happen after componentDidMount yet? Does it have to do with the fact that you make everything fit inside of a frame? Also if I have access to the dom reference in componentDidMount and I can read from the dom eg. computed values I'd expect layout to be done at least 🤔

[giuseppeg]

i mean aren't mutations synchronous as well (i.e. might trigger a layout/paint)?

[bvaughn]

Also if I have access to the dom reference in componentDidMount and I can read from the dom eg. computed values I'd expect layout to be done at least

Reading certain properties will force the browser to do sync layout before returning a value

[bvaughn]

Here's a list @paulirish created some time ago of which property reads trigger the browser to synchronously calculate the style and layout
https://gist.github.com/paulirish/5d52fb081b3570c81e3a

I access certain properties the browser could do layout before mine applies styles?

If React has mutated the DOM in such a way as to have invalidated layout or styles, and you force layout/reflow by reading one of these properties, then yeah– the browser would synchronously recalculate things.

[giuseppeg]

@bvaughn I figured out a way to replicate https://twitter.com/giuseppegurgone/status/1041319367886221312

[giuseppeg]

@bvaughn for this specific use case would it be ok to keep the side effect in render like this:

render() {
  const stringifiedProps = this.props.styleId + String(this.props.dynamic)
  if (this.stringifiedProps !== stringifiedProps) {
    this.stringifiedProps = stringifiedProps
    styleSheetRegistry.add(this.props)
  }
  return null
}

the comparison is a guard to make sure that styleSheetRegistry.add is only called once when the props don't change

[bvaughn]

All side effects are potentially dangerous.

In the case of updates, my best guess for when styles could safely be added is still getSnapshotBeforeUpdate– because it's part of the commit phase and it runs before the DOM is mutated or componentDidMount/componentDidUpdate get called. (This isn't what it was meant to be used for, but it could work.)

Unfortunately it wouldn't work for the initial render nor for the server-side rendering case. I don't really have a good solution for those to be honest.

[bvaughn]

I suggest you all might create an RFC for this.

This one I created a while ago could help with your server side rendering case but it wouldn't help with the client-side mount case:
reactjs/rfcs#8

[giuseppeg]

@bvaughn thank you, that's precious information!

[bvaughn]

I have some good news for you! We are working on something (no RFC yet– but hopefully soon) that should solve the mount+update mutation timing issue we've been discussing here. I hope to share more soon!

[giuseppeg]

aw nice, can't wait to see what you came up with! (happy to help if necessary)

[sebmarkbage]

I was looking at this code trying to figure out some concurrent mode compatibility issues. This fix is not concurrent mode safe (for example, it could remove things before the new changes have committed). However, we'd also expect this to cause bad perf with concurrent mode since you'd have more relayouts while React yields.

I was able to repro in React by simply adding a previous sibling like this (the codesandbox is broken for me right now so I can't fork it):

<div ref={n => { if (n) n.clientWidth; }} />

This forces a layout. This seems to trick Chrome into treating this as already mounted. Any subsequent mutations then cause a CSS transition to happen. What happens in this case is this: 1) React adds the DOM nodes to the document. 2) Something reads layout in a different life-cycle/effect. 3) React fires JSXStyle's life-cycle which then inserts the style sheet into the document. This causes Chrome to think this is an update to an already visible component and triggers a transition.

Now even if this was fixed in browsers, this sequencing would still leave the second step to potentially read the wrong layout.

We originally planned to add something like useMutationEffect which is what I think @bvaughn was referring to. Since then we decided not to do this. However, the alternative reactjs/rfcs#96 would not actually be sufficient for this case.

So this is missing a good solution atm.

[giuseppeg]

@sebmarkbage this is an intermediate patch, the final version where I tried to make it concurrent mode safe is here https://github.com/zeit/styled-jsx/blob/343795d3b2fef6d3d085921b0d4cf31b4d9be709/src/style.js but back then my understanding of all this was very poor and I haven't looked into it ever since.

[giuseppeg]

Leaving a link to a reproduction for future reference https://codesandbox.io/s/react-17-repaint-demo-forked-c716t?file=/src/App.js

[huozhi]

useInsertionEffect hook looks like can solve the issue mentioned above. Test in the codesandbox