Preact: 10.0.0-alpha.4 shouldComponentUpdate keeps Fragment's other children

Created on 17 Mar 2019  路  3Comments  路  Source: preactjs/preact

The issue happens when conditionally rendering a Component that renders multiple elements in a Fragment and also has a shouldComponentUpdate method. The state now changes to not render that component anymore, but the second and third children aren't removed (the text that was rendered as well does get removed).
Using no Fragments or removing shouldComponentUpdate fixes it.

The only two correct "page states" are (textarea + "Preview" + "This should never..." + "... neither this...") and ("Error!"):

bug

if (process.env.NODE_ENV === "development") {
    require("preact/debug");
}

import { h, render, Component, Fragment } from "preact";

class Preview extends Component {
    shouldComponentUpdate(nextProps, nextState) {
        return nextProps.output !== this.props.output;
    }

    render() {
        const { input, output } = this.props;
        return (
            <Fragment>
                Preview:
                <div>
                    This should never be displayed together with "Error"
                </div>
                <div>... and either this third element</div>
            </Fragment>
        );
    }
}

class App extends Component {
    constructor(props) {
        super(props);

        this.state = {
            input: 'console.log("A");',
            output: null,
            error: null
        };
    }

    run() {
        if ((this.state.input.split('"').length - 1) % 2 !== 0) {
            this.setState({
                error: true,
                output: null
            });
            return;
        }

        const output = "console.log('Hello');";
        this.setState({
            error: null,
            output
        });
    }

    render() {
        return (
            <div>
                <textarea
                    onInput={e => this.setState({ input: e.target.value })}
                >
                    {this.state.input}
                </textarea>
                <br />
                <button onClick={() => this.run()}>Run!</button>
                <hr />
                {this.state.error ? (
                    <div>Error!</div>
                ) : (
                    this.state.output && (
                        <div>
                            <textarea>{this.state.output}</textarea>
                            <br />
                            <Preview
                                input={this.state.input}
                                output={this.state.output}
                            />
                        </div>
                    )
                )}
            </div>
        );
    }
}
render(<App />, document.getElementById("root"));

Codesandbox: https://codesandbox.io/s/mq65lo49kx

Most helpful comment

I took a look at this and it turns out my changes originally in #1515 didn't fix it, but I updated my PR to include the proper fix and I also included a slightly modified version (so it's testable) of the codesandbox linked in the description of this issue in our test suite.

Cheers!

All 3 comments

The description of https://github.com/developit/preact/pull/1484 describes this issue quite well (though in this case, Fragments are part of the problem); unfortunately, that PR didn't fix it.

... it lead to leaving old DOM nodes around whenever shouldComponentUpdate returns false.

Indeed, the other PR doesn't fix this issue. Seeing that Fragments are involved it's likely an issue with their reconciliation. The recent PR #1515 by @andrewiggins addresses most of not all of them.

Will check again tomorrow (midnight here).

I took a look at this and it turns out my changes originally in #1515 didn't fix it, but I updated my PR to include the proper fix and I also included a slightly modified version (so it's testable) of the codesandbox linked in the description of this issue in our test suite.

Cheers!

Was this page helpful?
0 / 5 - 0 ratings