react-treebeard icon indicating copy to clipboard operation
react-treebeard copied to clipboard

Mutates the state directly in the example.

Open jojo-tutor opened this issue 7 years ago • 2 comments

treebeard

In the example, app.js, line 60. You mutated directly the cursor object in the state. This is not a good practice and should not be done in react. The code block says this:

onToggle(node, toggled) {
        const {cursor} = this.state;

        if (cursor) {
            cursor.active = false;
        }

        node.active = true;
        if (node.children) {
            node.toggled = toggled;
        }

        this.setState({cursor: node});
    }

The correct way should be this:

onToggle(node, toggled) {
        const cursor = { ...this.state.cursor };

        if (cursor) {
            cursor.active = false;
        }

        node.active = true;
        if (node.children) {
            node.toggled = toggled;
        }

        this.setState({cursor: node});
    }

In this case it copies the cursor object itself before changing it.

jojo-tutor avatar Feb 16 '18 00:02 jojo-tutor

you should look at the underlying code and understand what the setState does .. which is NOTHING but forcing a re-render of your component. It would have the same effect if you just setState to be a random string on each onToggle. What is more shocking is that the <Treebeard> component mutates its data prop ! The Treebeard component works, but boy does it break some React rules ...

andrewlorenz avatar Jul 09 '18 14:07 andrewlorenz

@andrewlorenz , yeah, I agree. This will actually work, especially by using React.Component, but it breaks the rule of react itself, sooner or later, this may create a serious bug and it will be hard for you to debug it when your application grows big and full of anti-patterns.

jojo-tutor avatar Jul 10 '18 13:07 jojo-tutor