Mithril.js: unloadCachedControllers usage of noop overwrites $old

[leiyangyou]

found a bug where the wrong onunload has been assigned to controllers, we tracked it down to the following lines

function unloadCachedControllers(cached, views, controllers) {
        if (controllers.length) {
            cached.views = views
            cached.controllers = controllers
            forEach(controllers, function (controller) {
                if (controller.onunload && controller.onunload.$old) {
                    controller.onunload = controller.onunload.$old
                }

                if (pendingRequests && controller.onunload) {
                    var onunload = controller.onunload
                    controller.onunload = noop
                    controller.onunload.$old = onunload
                }
            })
        }
    }

$old on noop gets overwritten when multiple controllers are involved

[isiahmeadows]

Sounds like changing noop to function () {} so they aren't referencing the same thing may help. Try that, and if it works, a PR with tests will be quickly accepted.

[lhorie]

@leiyangyou can you provide a jsfiddle demonstrating the issue?

[leiyangyou]

I was having a hard time trying to construct an example.

The attached fiddle demonstrates a behaviour where the onunload chain is not always called properly, (and is unrelated to the reported bug), can someone verify if the behaviour is intended?

https://jsfiddle.net/leiyangyou/tjdjfba9/7/

what it basically does is mounting distinct components on the same node 3 times, but it seems like the nested components only gets unloaded on the first unload

console output is as follows:

mounting a
mounting b
inner controller unload a
outer controller unload a
mounting c
outer controller unload b

[leiyangyou]

@isiahmeadows we had an issue that wasn't more complicated, and is related to pendingRequests,

and we were able to resolve it by changing noop to function () {}

[isiahmeadows]

@leiyangyou Could you make a PR out of it? It'd be much appreciated.

[stffrd]

We can close this now, yeh?

[tivac]

LGTM.