componentWillUnmount is lazy?

[Download]

I'm turning here because I ran into this issue and I have real difficulty explaining it. I must admit I don't know what I am doing exactly because I am porting someone else's work and tests, but it was supposed to be an easy port and is turning out... not so easy... I am not using preact-compat because I feel a true port should not need it.

I pushed my work so far so you can see what I'm doing: https://github.com/Download/preact-helmet/tree/port-to-preact

The problem: I have a test failing in preact-helmet and I can't explain it. It looks like componentWillUnmount is called later than expected by the test. See the Travis run.

In the test I am forcing an unmount by rendering an EmptyComponent. This is based on remarks from the issue tracker here and from preact-compat.

In each test, I render using this code:

child = render(
  <Jsx>Bla</Jsx>
  ,
  container, child
);

Here, container is a div nested in the document body, that is never replaced. Child is initially undefined and then the result of each render call.

In an after hooks that runs after each test, I call unmountComponentAtNode, which I have tweaked slightly from the one found in preact-compat:

function unmountComponentAtNode(container, child) {
    return render(<EmptyComponent />, container, child);
    function EmptyComponent() { return null; }
}

Analysis so far: I debugged this the ugly way: by placing console.info statements in the code.

Firs I placed a log statement in unmountComponentAtNode. That confirms that this function is indeed called after each test.

Next I placed two log statements in preact-side-effect, that this module depends on. I just modified the local code that can be found in ./node_modules/preact-side-effect/lib/index.js. One log statement in componentWillMount and one in componentWillUnmount. The one in componentWillMount also logs the new count (it is keeping track of a stack of mounted instances). Here is the code for completeness:

      SideEffect.prototype.componentWillMount = function componentWillMount() {
        console.info("SideEffect.componentWillMount: mountedInstances.length=", mountedInstances.length + 1);
        mountedInstances.push(this);
        emitChange();
      };

      SideEffect.prototype.componentWillUnmount = function componentWillUnmount() {
        console.info("SideEffect.componentWillUnmount");
        var index = mountedInstances.indexOf(this);
        mountedInstances.splice(index, 1);
        emitChange();
      };

The weird thing is that at the start of a new test, if I call SideEffect.peek(), it still gives me the state from the last mounted instance. And indeed I see in the logs that at this point, componentWillUnmount has not been called yet (even though unmountComponentAtNode has). If I then proceed to render again in this new test, componentWillUnmount is called just before the new node is mounted. Here is the log output:

Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √tags without 'src' will not be accepted
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'SideEffect.componentWillUnmount'
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'SideEffect.componentWillMount: mountedInstances.length=', 1
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'SideEffect.componentWillMount: mountedInstances.length=', 2
        √will set script tags based on deepest nested component
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'SideEffect.componentWillUnmount'
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'SideEffect.componentWillUnmount'
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'SideEffect.componentWillMount: mountedInstances.length=', 1
        √sets undefined attribute values to empty strings
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √won't render tag when primary attribute (src) is null
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √won't render tag when primary attribute (innerHTML) is null
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
      noscript tags
        √can update noscript tags
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √will clear all noscripts tags if none are specified
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √tags without 'innerHTML' will not be accepted
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √won't render tag when primary attribute is null
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
      style tags
        √can update style tags
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √will clear all style tags if none are specified
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √tags without 'cssText' will not be accepted
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
        √won't render tag when primary attribute is null
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
    misc
      √throws in rewind() when a DOM is present
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
      √lets you read current state in peek() whether or not a DOM is present
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
      √will html encode string
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
      √will not change the DOM if it is receives identical props
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
      ×will only add new tags and will preserve tags when rendering additional Helmet instances
        AssertionError: expected { Object (linkTags) } to have a property 'metaTags'
            at Context.<anonymous> (webpack:///lib/test/HelmetTest.js:9:77606 <- lib/test/HelmetTest.js:235:77628)

Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
      √can not nest Helmets
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
      √will recognize valid tags regardless of attribute ordering
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'unmountComponentAtNode: child is defined'
Chrome 56.0.2924 (Windows 10 0.0.0) INFO LOG: 'SideEffect.componentWillMount: mountedInstances.length=', 1

(sorry about the noise, that's Karma)

The last test is the failing one. I'm showing all this logging because it shows how other tests (that are doing rendering) actually successfully mount/unmount... which makes this one test so weird. Also notice how the length reported from componentWillMount (which is the new length after mounting) is correct. Instances are not piling up. Also notice how componentWillUnmount is called just before the new one is mounted, even though unmountComponentAtNode has sometimes been called multiple times in between.

I have been staring at this for hours now and can't figure out what is happening so a second pair of eyes would be really appreciated!

[Download]

Oh by the way, in case it wasn't clear, I do not think this is a bug in Preact. I'm pretty sure I messed something up myself... I just really can't explain the magic that happens between the unmountComponentAtNode calling render and the componentWillUnmount method being called.

[developit]

@Download quick fix: can you try making EmptyComponent a class?

class EmptyComponent {
  render(){ return null }
}

[Download]

It gives completely different behavior! A lot of tests are failing now... No idea why there is so much impact! I'll have to dig a bit more... I must say I am tempted to try empty string next... but dinner calls!

I'll update later tonight. Thanks for this tip it seems very relevant.

[developit]

Ack, sorry haha.

[Download]

Ok, so I tried with empty string and it fixes all my tests.

Somehow not very comforting but I think I'm going to give up on trying to understand it for now.

[Download]

I was a bit too quick. The problematic test was still disabled. It still gives the same issue.

Recap:

Using a functional EmptyComponent, or empty string, give this one failing test. Using a class component gives a whole bunch of failures. So many actually that I quickly reverted it.

[developit]

I may have run into something like this today. My workaround for now (need to investigate) was to switch from passing child (like in your unmountComponentAtNode) to using container.lastChild.

[Download]

container.lastChild?

I had tried firstChild but it didn't help. Will try lastChild later but I'm tired now and have other work still so it will have to wait.

Normally unmounting works great. This attempt to force it is because it happens in a test. But it would be great if it was clear how to do it.

[developit]

Any chance it's related to #538?

[Download]

@developit Mmmm yes this definitely seems related.

I think, given there are open issues on this, it might be worthwile for you to clone my repo and run the tests (uncomment the failing one first). Then you can experiment with the different ways of forcing the unmount and see the different behaviors first hand.

I can imagine you have other things to do though!

I'm going to leave it for now with the uncommented test and first start actually using the ported component. I'll have to see whether it's going to be an issue in the app itself.

Do you want to keep this issue open for now?

[developit]

Let's keep this issue open, but I'll be fixing #538 first - after that's merged we'll check if this still needs separate work.

[developit]

Fixed in 8.