Attached and detached events not firing when expected when conditionally rendering JSX children.

[ethib137]

I have written a tab component that works by only rendering one of it's child components at a given time (https://github.com/liferay/liferay-plugins-ee/blob/ee-6.2.x/portlets/loop-portlet/docroot/js/src/components-metal/Tabs.js#L66).

I would expect that each time I switch a tab, the previous child component would be "detached", and the new child component would be "attached".

What is actually happening is that the "attached" event is only running the first time the child component is rendered. After I have viewed a child component once, even if I switch to a different tab and come back, the "attached" event will not fire a second time. Also, the "detached" event is never fired.

Please let me know if I am thinking about the lifecycle correctly or if you need any clarifications.

Thanks.

[mairatma]

You're right that these lifecycle events/functions should be fired in this case. I can see from looking at the code why attached is only being called for the first time, but not sure about what's causing detached not to work yet. I'll investigate more and fix this.

[ethib137]

Thanks Maira!

On Wed, May 25, 2016 at 10:16 AM, Maira Bello [email protected]
wrote:

You're right that these lifecycle events/functions should be fired in this
case. I can see from looking at the code why attached is only being
called for the first time, but not sure about what's causing detached not
to work yet. I'll investigate more and fix this.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#115 (comment)

Evan Thibodeau

UI Software Engineer
Liferay, Inc.

[mairatma]

@ethib137, I investigated a bit and it seems like the lifecycle functions are actually being called at the right time.

There are two things that may be happening in your case:

1. We may be reusing the same component instance for different tabs. If the component constructor is the same for different tabs we'll reuse the instance when you switch tabs, since we can just update the component's data and rerender it to get the final results. That's better for performance than creating a new instance every time. In this case we're just rerendering, so no detached or attached should be called. The one that would be called is rendered. You can check if that's the case by inspecting the value received by the rendered lifecycle function (or event). If gives you a flag indicating if this is the first render or not.
2. We may be using different component instances whenever tabs are switched. If the component constructor is different for different tabs, we'll always dispose of the previous tab instance (since it's not being rendered anymore) and create a new instance for the tab to be rendered. We won't ever reuse tab instances in this case, and so attached would only be called on first render since they won't be added back later. So if you're listening to events on an instance that was created for the first tab, those events will only be valid until it's switched out. When switched back in another instance is used, so you need to listen to the events on the new one instead. Not sure why you wouldn't see the detached event firing in this case though, so I think that your case is probably 1.

[bryceosterhaus]

@mairatma so am I understanding it correctly in saying that the detached lifecycle will be executed only if I explicitly call detach or dispose?

Cause I am testing use case 2 and I am not seeing detached event ever firing. I would expect to see detached fire anytime that component is removed from the DOM.

[bryceosterhaus]

Also, say I have two components, C1 and C2.

<C1>
     <C2 />
</C1>

If I dispose C1, will C2 also be disposed?

[mairatma]

No, both the detached event and lifecycle function should be executed whenever the component is removed from the dom by a parent component, like in the case that @ethib137 explained. I added a test for a case like this yesterday and it seemed to be working correctly: https://github.com/metal/metal-incremental-dom/blob/master/test/IncrementalDomRenderer.js#L982.

Can you show me the code you're testing this with so I can try to investigate if it's some kind of bug?

And yes, when a parent component is disposed, its child components are disposed as well (or at least they should be).

[bryceosterhaus]

Go ahead and try this. You will notice that detached is never fired and attached will be fired any time ChildComponent becomes visible.

[bryceosterhaus]

Actually I just cleared out and re-installed latest npm modules and it seems to work now.

Let me test in Loop specifically and get back to you.

[bryceosterhaus]

okay I was able to reproduce outside of loop using. Issue seems to be specifically when passing through children

Components:

import Component from 'metal-jsx';

class ParentComponent extends Component {
    onClick() {
        this.active = !this.active;
    }

    render() {
        return (
            <div>
                <button data-onclick={this.onClick.bind(this)}>hide child</button>

                {this.active &&
                    this.config.children
                }
            </div>
        );
    };
}

ParentComponent.STATE = {
    active: {
        value: true
    }
}

export class ChildComponentTwo extends Component {
    attached() {
        console.log("child-attached");
    }
    detached() {
        console.log("child-detached");
    }

    render() {
        return (
            <div>
                child
            </div>
        );
    };
}

export default ParentComponent;

render via

<ParentComponent>
    <ChildComponentTwo />
</ParentComponent>

[mairatma]

Oh I see now, that makes sense, thanks a lot for the detailed example! I'll work on fixing this.

[mairatma]

This should be fixed now, just update the dependencies :)

Let me know if there's any other case where these lifecycle events aren't being fired. I believe I've covered everything, and I've added test cases to make sure these problems don't come back.

[bryceosterhaus]

Thanks @mairatma!