sanity-io / block-content-to-react

Deprecated in favor of @portabletext/react
https://github.com/portabletext/react-portabletext
MIT License
161 stars 25 forks source link

Custom type renderers are not reused, but mounted/unmounted excessively #6

Closed vramdal closed 6 years ago

vramdal commented 6 years ago

I see a lot of unmounting/re-mounting of my component that is used in a custom serializer. In my mind, the component should not be unmounted/remounted on every props update, but instead just updated with new props.

Given the following app (or see sandbox):

import ReactDOM from 'react-dom';
import React, {Component} from "react";
import BlockContent from "@sanity/block-content-to-react";

class Sometype extends Component {

    componentDidMount() {
        console.log("Sometype.componentDidMount - this should only happen once", arguments);
    }

    componentWillReceiveProps(nextProps) {
        console.log("Sometype.componentWillReceiveProps - this should happen on every click, but never happens", arguments);
    }

    render() {
        return <p onClick={this.props.clickHandler}>Click me! {this.props.children}</p>
    }
}

const block = {
    "_key":"58cf8aa1fc74",
    "_type":"sometype",
    "something":{someProperty: "someValue"}
};

class App extends Component {
    constructor() {
        super();
        this.state = {clicks: 0};
        this.serializers = {
            types: {
                sometype: props => (
                    <Sometype clickHandler={() => this.setState({clicks: this.state.clicks + 1})}>
                        {props.node.something.someProperty + " " + this.state.clicks}
                    </Sometype>
                )
            }
        };
    }

    render() {
        return (
            <div className="App">
                <BlockContent key={'blockcontent-1'} blocks={[block]} serializers={this.serializers}/>
            </div>
        );
    }
}

ReactDOM.render(<App />, document.getElementById('root'));

The app uses BlockContent with a map of a single serializer for type sometype. The serializer renders a Sometype component.

In the browser, when clicking any content from Sometype, the state of the container component is updated, thus triggering a re-render.

At this point, I would expect Sometype.componentWillUpdate to be called, with the new updated props. Instead, the Sometype instance is unmounted, and a new instance is mounted, thus never calling componentWillUpdate. I'm not sure this violates any spec of a React component, but it is certainly uncommon behaviour.

Compare to an app not using BlockContent, where Sometype.componentWillUpdate is called on props update.

By modifying block-content-to-hyperscript/lib/serializers.js so that it returns an object instead of a function (thus hardcoding React.createElement instead of passing it as a parameter to the function), and modifying BlockContent to used the exported object instead of calling the function, the problem goes away, and Sometype.componentWillUpdate is called as expected.

This article describes a somewhat similar problem, but I am unable to see how it can apply to this problem.

vramdal commented 6 years ago

The problem seems to be related to how block-to-hyperscript's serializers.js returns a new serializer function on every occation.

By saving and reusing the serializer function, the problem can be avoided. But surely there must be a better way.

vramdal commented 6 years ago

BlockContent already gets defaultSerializers from calling getSerializers(renderNode). If we let it also store a reference to serializeSpan, we could pass those two functions to blocksToNodes, so that it would not have ot call getSerializers again:

BlockContent.js:

const renderNode = React.createElement
- const {defaultSerializers} = getSerializers(renderNode)
+ const {defaultSerializers, serializeSpan} = getSerializers(renderNode)

const SanityBlockContent = props => {
-  return blocksToNodes(renderNode, Object.assign({blocks: []}, props))
+  return blocksToNodes(renderNode, Object.assign({blocks: []}, props), defaultSerializers, serializeSpan)
}

blocksToNodes.js:

- function blocksToNodes(h, properties) {
+ function blocksToNodes(h, properties, defaultSerializers, serializeSpan) {
-   const {defaultSerializers, serializeSpan} = getSerializers(h)

WDYT? If this seems like a good way to go, I could try putting together a PR.

rexxars commented 6 years ago

Thanks for a thorough bug report and for looking into it so deeply!

I think your solution seems like a good starting point seeing as it doesn't change the public API - would love a PR.

Looking at the code now, I think perhaps we should try to refactor things a bit so that the serializers are not created on each iteration, it seems excessive and costly - but I suppose we can do this later.

rexxars commented 6 years ago

Thanks again for reporting and for the fix! Really appreciated! Fixed in v1.3.7

ibrhm-mnf-fingent commented 3 years ago

@rexxars @vramdal Is this issue still exist in block-content-to-react? Custom serializer component unmounting and mounts again on rerendering.

Codesandbox example updating a simple state after 3 seconds to show custom serializer component unmounting and remount. https://codesandbox.io/s/sad-tu-fjgty?file=/src/App.js