Preact: inconsistency during unmount

[kruczy]

Hey,
there is an inconsistency during componentDidUnmount, most of the time this.nextBase is populated but sometimes this.base will and this.nextBase is null

don't have a repro yet, it happens in many places in my app and in different components

could be the cause of this is similar as the one that prevented me from using any version from beta19+

will provide a repro if I manage to figure out how to break it outside of my app

[developit]

@kruczy I think I might know why this is, just trying to figure out the semantics. There is an unmount flow that does not remove the component, but it's possible we should just always be removing when unmounting. Repro would help, or I can give you a trial version with unmount always triggering remove.

[kruczy]

your comment gave me an idea, made a repro :)

http://jsfiddle.net/pstma6fj/

[developit]

Ahh, interesting - high order components have their base left in-tact! Good find.

[kruczy]

I have also noticed that componentDidUnmount gets called two times though the component is unmounted only once

[developit]

@kruczy only for high-order components?

[kruczy]

sorry my bad, its not really called twice, was just looking at it wrong

added more lifecycle stuff to the example, maybe it will help with @Madumo case:
EDIT: http://jsfiddle.net/pstma6fj/5/

[kruczy]

http://jsfiddle.net/pstma6fj/6/

curiously, componentWillMount sometimes sometimes is null, null sometimes null, element

[developit]

this.nextBase we can mostly ignore - it's internal (should perhaps be underscore-prefixed). I'm migrating your fiddle to a unit test right now so I can test what I believe is a proper fix!

[kruczy]

hmm, the question is then how do I cleanup this.base before it is taken out of the component to be reused?

I need to do this as I am calling addEventListener on it, any thoughts?
Thanks

[developit]

Easiest, if possible, is to use props to attach events, so they get auto-removed on next use if they aren't needed. For the root, I'd actually suggest using a ref:

class Foo extends Component {
  componentWillUnmount() {
    this.dom.removeEventListener( ... );
  }
  render() {
    return <div ref={ c => this.dom = c }>foo</div>;
  }
}

With the bug fix in place, you should always be able to rely on this.base within componentWillUnmount(). this.base will generally have already been nulled by the time componentDidUnmount() is called.

[kruczy]

Ya, cant really do that as this a component that does not really know what it is rendering, I could wrap every thing in a div but it feels dirty...

tried using cloneElement to add an on... but if the top level element is a component it does not work

ill, try to use componentWillUnmount instead of componentDidUnmount to cleanup this.base, it should work as it is called before anything else

[developit]

Correct, it fails for children that are themselves components (wasn't sure your use-case). Definitely use componentWillUnmount() - the fact that this.base is still set sometimes during componentDidUnmount() is actually a small bug.

[kruczy]

ya, didn't realise I should not use this.base in componentDidUnmount, Thanks :+1:

hopefully this solves my problems :)

[developit]

hopefully! Plus the fix on my end (will publish as soon as I can, likely tonight)

[developit]

I modified your example to only log the error case, and verified that with the fix in place (5.5.0), the error case is never triggered:
http://jsfiddle.net/developit/b0ymwy6c/