data prop is mutated, very poor implementation and React anti-pattern

[andrewlorenz]

This component - and the example provided for it - does 2 very naughty things:

1. it requires your component to be a full React.Component and use state/setState - but this is actually only required to force your component to re-render. And it needs this because
2. it mutates your data prop as it is an object, and is passed by reference.

In order to get past linters, and to avoid warnings/errors from React itself, it 'hoodwinks' them into not noticing that it is doing 2. It does this using the following code in its implementation:

class TreeBeard extends React.Component {
    render() {
        const {animations, decorators, data: propsData, onToggle, style} = this.props;
        let data = propsData;

see how this.props is de-structured as usual, but data is renamed to propsData, then promptly assigned to a new variable called data (all the time being an object, and thus all the time being a reference to 'data' from your own component).

This component should be rewritten to - at the very least - hide the implementation details from the user - but preferably to not mutate props in the first place.

[therealparmesh]

+1

FWIW I tried to kindle a conversation around this with a rough draft of a PR before the maintainers transferred the repository and I didn’t get anywhere.

https://github.com/storybooks/react-treebeard/pull/114

[youpiwaza]

Mutating data also provokes serious unicity issues when using multiples instances of Treebeard.

Having several datas like so

import data from './data.json';
const data1 = Object.assign({}, data);
const data2 = Object.assign({}, data);

[...]

<Treebeard data={data1} onToggle={this.handleToggleHOF('tree1')} />
<Treebeard data={data2} onToggle={this.handleToggleHOF('tree2')} />

doesn't work, as both trees are updated on any change. It also updates the original data..

Right now I found a workaround by duping the datas (ex: another import of duped "data2.json") & having multiples imports seems to work, but it's far from ideal :/

Edit : Here's my repo with both the problem and the workaround illustrated. https://github.com/youpiwaza/react-boilerplate-test-treebeard I hope you'll manage to clean your plugins, I still like a lot to use it.

Cheers

[Loffe]

This problem is also visible in the examples. For me, it is not really visible how onToggle updates the state to be rendered. It updates the state with cursor, which is not used in the render method.

This "works" only because the object data is implicitly mutated, and setState() causes a re-render to occur.

class TreeExample extends React.Component {
    constructor(props){
        super(props);
        this.state = {};
        this.onToggle = this.onToggle.bind(this);
    }
    onToggle(node, toggled){
        if(this.state.cursor){this.state.cursor.active = false;}
        node.active = true;
        if(node.children){ node.toggled = toggled; }
        this.setState({ cursor: node });
    }
    render(){
        return (
            <Treebeard
                data={data}
                onToggle={this.onToggle}
            />
        );
    }
}