Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metal incorrectly handles reuse of elements when conditionally rendering #147

Closed
ethib137 opened this issue Aug 5, 2016 · 12 comments
Closed
Assignees

Comments

@ethib137
Copy link

ethib137 commented Aug 5, 2016

import Component, {Config} from 'metal-jsx';

class ComponentOne extends Component {
    created() {
        console.log('created', this);
    }

    disposed() {
        console.log('disposed', this);
    }

    render() {
        return <div>{'Component One'}</div>;
    }
}

class ComponentTwo extends Component {
    created() {
        console.log('created', this);
    }

    disposed() {
        console.log('disposed', this);
    }

    render() {
        return <div>{'Component Two'}</div>;
    }
}

class TestKit extends Component {
    created() {
        this.handleClick_ = this.handleClick_.bind(this);
    }

    handleClick_() {
        this.state.visible_ = !this.state.visible_;
    }

    render() {
        return (
            <div>
                <a href="javascript:;" onClick={this.handleClick_}>{'Toggle Component One'}</a>

                {this.state.visible_ &&
                    <div>
                        <ComponentOne />
                    </div>
                }

                <div>
                    <ComponentTwo />
                </div>
            </div>
        );
    }
}

TestKit.STATE = {
    visible_: Config.value(true)
};

export default TestKit;

In this test case, when you click Toggle Component One you would expect that ComponentTwo would be removed from the page, and that only that component would be disposed.

What actually happens is that ComponentTwo is recreated, and both ComponentOne and ComponentTwo are disposed.

This can be avoided by adding a key to the div that contains ComponentTwo, but it seems like it shouldn't always be necessary to add keys to all the succeeding div's after an element that is conditionally rendered.

I'm unsure if there is a better way to handle this, but I thought it would be helpful to start the discussion none the less.

Let me know if you have any questions.

Thanks.

@mairatma
Copy link
Contributor

mairatma commented Aug 8, 2016

Yes, I believe there is a better way to handle this, will look into it.

@mairatma mairatma self-assigned this Aug 8, 2016
@ethib137
Copy link
Author

Hey @mairatma, I was wondering if there was any update on this issue? Thanks.

@mairatma
Copy link
Contributor

Hey @ethib137. I was out on vacation the past couple of weeks and didn't have enough time to work on this before it started, but will go back to it now that I'm back :)

@ethib137
Copy link
Author

Thanks @mairatma! Glad to have you back :)

@ethib137
Copy link
Author

hey @mairatma, I just upgraded to metal-jsx 2.4.0 and I'm still having an issue with this use case, it's just slightly different. Now when you hide componen1, it creates a new component2 and then disposes component1 and the old component2.

What's different, is that this only happens the first time I hide component1. After that, it behaves correctly only disposing and creating component1.

@ethib137
Copy link
Author

@mairatma can we reopen this?

@mairatma
Copy link
Contributor

Are you trying this with the same example you gave above? I just did and it behaved as expected, disposing only component1 from the first time I clicked the button.

@ethib137
Copy link
Author

Yeah, I'm using the same code... that's weird, that it isn't reproducible for you... let me double check.

@ethib137
Copy link
Author

Hmm, I reinstalled all my node modules and I still run into the same issue the first time I toggle component1.

@ethib137
Copy link
Author

I just had @kienD try it and he sees the same issue.

@mairatma
Copy link
Contributor

We confirmed that this works well if this component is on the root, but on @ethib137's use case it's inside some other parents. We'll investigate more, but I'm reopening until we figure out if the problem is being caused by a similar issue or not.

@mairatma mairatma reopened this Aug 31, 2016
@ethib137
Copy link
Author

Hey @mairatma can you give this a try, I think it just needed one more container.

import Component, {Config} from 'metal-jsx';

class ComponentOne extends Component {
    created() {
        console.log('created', this);
    }

    disposed() {
        console.log('disposed', this);
    }

    render() {
        return <div>{'Component One'}</div>;
    }
}

class ComponentTwo extends Component {
    created() {
        console.log('created', this);
    }

    disposed() {
        console.log('disposed', this);
    }

    render() {
        return <div>{'Component Two'}</div>;
    }
}

class ComponentThree extends Component {
    created() {
        this.handleClick_ = this.handleClick_.bind(this);
    }

    handleClick_() {
        this.state.visible_ = !this.state.visible_;
    }

    render() {
        return (
            <div>
                <a href="javascript:;" onClick={this.handleClick_}>{'Toggle Component One'}</a>

                {this.state.visible_ &&
                    <div>
                        <ComponentOne />
                    </div>
                }

                <div>
                    <ComponentTwo />
                </div>
            </div>
        );
    }
}

ComponentThree.STATE = {
    visible_: Config.value(true)
};

class TestKit extends Component {
    render() {
        return (
            <div>
                <ComponentThree />
            </div>
        );
    }
}

export default TestKit;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants