React: Bug: Horrifying performance in React experimental

[Jack-Works]

React version: 0.0.0-experimental-33c3af284

Tested: 0.0.0-experimental-e5d06e34b is okay, 0.0.0-experimental-33c3af284 is broken

Steps To Reproduce

1. It's in a complicated application so not sure how to reproduce
2. If the React team has an interest in it, I will try to provide it.

React-experimental took 14 seconds to handle a button click

[gaearon]

Sounds odd. A repro would be helpful.

Have you looked at where the time is being spent percent-wise?

[Jack-Works]

I can reproduce it in our product but our product is very complicated. I'll try to repro it but I can't guarantee it.

[Jack-Works]

Run the command yarn webpack-dev-server -o index.js with the following files. This includes a third party dependency @material-ui/core and our hacky code to run React and JSS in the ShadowDOM. But it works in 0.0.0-experimental-e5d06e34b and not in 0.0.0-experimental-33c3af284 so I'm curious why.

index.html

<div id="app"></div>
<script src="/index.js"></script>

package.json

{
  "dependencies": {
    "@material-ui/core": "^4.9.14",
    "react": "0.0.0-experimental-33c3af284",
    "react-dom": "0.0.0-experimental-33c3af284",
    "use-subscription": "^1.4.1"
  },
  "devDependencies": {
    "webpack": "^4.43.0",
    "webpack-cli": "^3.3.11",
    "webpack-dev-server": "^3.11.0"
  }
}

src/index.js

import React from "react"
import { useSubscription } from "use-subscription"
import ReactDOM from "react-dom"
import { Dialog, StylesProvider, jssPreset, Chip } from "@material-ui/core"
import { create, SheetsRegistry } from "jss"
const c = React.createElement

class InformativeSheetsRegistry extends SheetsRegistry {
    constructor() {
        super(...arguments)
        this.callback = new Set()
    }
    inform() {
        // ? Callback must be async or React will complain:
        // Warning: Cannot update a component from inside the function body of a different component.
        setTimeout(() => {
            // TODO: batch update
            // ? aggregating multiple inform request to one callback is possible
            for (const cb of this.callback) cb()
        })
    }
    addListener(cb) {
        this.callback.add(cb)
        return () => void this.callback.delete(cb)
    }
    add(...args) {
        super.add(...args)
        this.inform()
    }
    reset(...args) {
        super.reset(...args)
        this.inform()
    }
    remove(...args) {
        super.remove(...args)
        this.inform()
    }
}

let proxy
const handler = {
    // ! (1) to make it more like a Document
    // ! (2) to make it more like an Element
    get(target, property, handler) {
        // ! (1) make react move event listeners to shadowroot instead of document
        if (property === "ownerDocument") return handler

        // ! (2) if it's a function, use ours
        const val = Reflect.get(target, property, target)
        if (typeof val === "function") return val.bind(target)

        // @ts-ignore
        // ! (1) document can createElement, createElementNS, etc., so if we don't have such methods, use theirs
        if (
            typeof val === "undefined" &&
            typeof document[property] === "function"
        )
            // @ts-ignore
            return document[property].bind(document)

        // ! (2) if it's not a function, use theirs
        // @ts-ignore
        return document.body[property]
    },
    set(target, property, value) {
        return Reflect.set(target, property, value, target)
    },
}
const div = document.createElement("div")
const portalShadowRoot = div.attachShadow({ mode: "closed" })
document.body.appendChild(div)
function PortalShadowRoot() {
    if (!proxy) proxy = new Proxy(portalShadowRoot, handler)
    return proxy
}

function ShadowRootDialog(_props) {
    const ref = React.useRef(null)
    const styles = useSheetsRegistryStyles(ref.current)
    // ? I need the render tree to get the shadowroot. Is the extra div must be rendered?
    // ? can style be transported to shadowroot directly instead of with dialog children?
    const { children, container, ...props } = _props
    return React.createElement(
        "div",
        { ref: ref },
        React.createElement(
            Dialog,
            Object.assign({}, props, {
                container: container || PortalShadowRoot,
            }),
            React.createElement("style", null, styles),
            children
        )
    )
}
const jssRegistryMap = new WeakMap()
const useSheetsRegistryStyles = (_current) => {
    const subscription = React.useMemo(() => {
        let registry = null
        if (_current) {
            // ! lookup the styled shadowroot
            const current = _current.getRootNode()
            if (current) {
                const shadowroot = current
                registry =
                    shadowroot === portalShadowRoot
                        ? null
                        : jssRegistryMap.get(shadowroot)
            }
        }
        return {
            getCurrentValue: () => registry && registry.toString(),
            subscribe: (callback) =>
                registry ? registry.addListener(callback) : () => 0,
        }
    }, [_current])
    return useSubscription(subscription)
}
class RenderInShadowRootWrapper extends React.Component {
    constructor(props) {
        super(props)

        this.proxy = new Proxy(this.props.shadow, {
            get(target, property) {
                if (property === "parentNode") {
                    const host = target.getRootNode({ composed: true })
                    if (host !== document) {
                        // ! severe error! The style cannot be managed by DOMRender
                        return null
                    }
                    return target
                }
                return target[property]
            },
        })
        this.jss = create({
            ...jssPreset(),
            insertionPoint: this.proxy,
        })
        this.registry = new InformativeSheetsRegistry()
        this.manager = new WeakMap()
        jssRegistryMap.set(props.shadow, this.registry)
    }
    render() {
        return (
            // ! sheetsRegistry: We use this to get styles as a whole string
            // ! sheetsManager: Material-ui uses this to detect if the style has been rendered
            c(StylesProvider, {
                sheetsRegistry: this.registry,
                jss: this.jss,
                sheetsManager: this.manager,
                children: this.props.children,
            })
        )
    }
}
function Changer() {
    const [s, ss] = React.useState(false)
    return c(ShadowRootDialog, {
        children: c(Chip, {
            label: "Try to click me 5 times",
            checked: s,
            onClick: () => ss(!s),
        }),
        open: true,
    })
}
ReactDOM.render(
    c(RenderInShadowRootWrapper, {
        shadow: portalShadowRoot,
        children: c(Changer),
    }),
    portalShadowRoot
)

[gaearon]

If you remove the shadow DOM stuff, is there still a problem?

[Jack-Works]

If you remove the shadow DOM stuff, is there still a problem?

No. But ShadowDOM is very, very important in our product

[gaearon]

It seems like to further narrow it down you'll want to see how the behavior of this hack differs between versions of React. I can try to find time to look at it later but since this is very unconventional you'll probably have better luck narrowing it down.

[eps1lon]

Something is exponentially escalating. When profiling in development the first click creates 16 commits, the second 64 and the third one 256. Not related to the Chip component but as soon as I replace the Dialog from Material-UI with a div it no longer degrades.

[eps1lon]

Repro with just React: https://codesandbox.io/s/distracted-shamir-7bx54

Clicking the button causes the click handler to run multiple times. Each click doubles the number of times the click handler runs.

As far as I can tell this is either an issue with ReactDOM.createPortal or that custom proxy over portalShadowRoot. The getter of that proxy returns return document.body[property]; at the end which seems sketchy given a shadow-root.

-return document.body[property];
+return portalShadowRoot[property];

fixes the perf issue.

Switching from legacy mode to concurrent mode has no observable effect.

[Jack-Works]

Thanks @eps1lon I will try your fix tomorrow. But does it possible to improve the usage of ShadowRoot so I don't need those dirty hack?

[eps1lon]

I've never used react with the shadow DOM so I can't help you with that.

[Jack-Works]

I feel the inconvenience from every aspect of the React ecosystem when using Shadow Dom.

React doesn't support it (event propagation bug)

JSS doesn't support it (can't inject style into shadow root)

MUI dialog doesn't support it well (it is using findDOMNode which doesn't support Shadow Dom)

I need to hack everything and I really hope to see the changes happen.😅

[eps1lon]

MUI dialog doesn't support it well (it is using findDOMNode which doesn't support Shadow Dom)

Only if your child components are class components and don't forward their ref to their outermost DOM node.

[Jack-Works]

(Code in @material-ui/core, TrapFocus)

Hmm, another example. IIRC, when rendering in the ShadowDOM, doc.activeElement will be the root element of ShadowDOM.

// DOM tree
<div>
    <!-- Shadow DOM -->
        <main> <!-- React root -->
            <input />
        </main>
    <!-- Shadow DOM End -->
</div>

When focusing on the input, the active element will be the div therefore it is buggy in the ShadowDOM and I have to disable all of this

[Jack-Works]

-return document.body[property];
+return portalShadowRoot[property];

This doesn't work in our product (React complaining when findDOMNode) but with a little change, it works now.

const body = document.body[property]
const self = portalShadowRoot[property]
return body ?? self

But I'm still wondering the reason behind the performance fall

[Jack-Works]

thanks @eps1lon

[stale[bot]]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[Jack-Works]

Bump, but I think it will be fixed by my pr

[Jack-Works]

I don't see this in our product recently. Maybe it's already fixed