When the route changes onunload is triggered for controllers that have already been unloaded

claudiofelber commented 8 years ago

Description

For every controller that has already been unloaded, the onunload handler is called again when another component is mounted (e.g. when the route changes).

Steps to Reproduce

In the subsequent example do the following:

Open the console
Click on settings link
Click on redraw a couple of times
Click on home link

You will see multiple UNLOAD messages in the console, one for every controller that has been created and already (!) been unloaded.

<!DOCTYPE html>
<html>
<body>
<script src="mithril.js"></script>
<script>
    var component = {
        controller: function() {
            console.log("CREATE");

            this.onunload = function() {
                console.log("UNLOAD");
            }
        },

        view: function(ctrl) {
            return m("div", [
                m("div", "Component"),
                m("a", {
                    href: "#",
                    onclick: function(e) {
                        e.preventDefault();
                        m.redraw();
                    }
                }, "Redraw")
            ]);
        }
    };

    var home = {
        view: function() {
            return m("div", [
                m("h1", "Home"),
                m("a", {
                    href: "/settings",
                    config: m.route
                }, "Settings")
            ]);
        }
    };

    var settings = {
        view: function(ctrl) {
            var noise = [];
            var count = parseInt(Math.random() * 10) + 1;
            for (var i = 0; i < count; i++) {
                noise.push(m("span", "Noise " + i  + " "));
            }

            return m("div", [
                m("h1", "Settings"),
                noise,
                m("div",
                    m("a", {
                        href: "/",
                        config: m.route
                    }, "Home")),
                m(component)
            ]);
        }
    };

    m.route.mode = "hash";
    m.route(document.body, "/", {
        "/": home,
        "/settings": settings
    })
</script>
</body>
</html>

Note: The random noise has been added to force the controller to be recreated.

Fix

Extend the unload() function in order to have it remove onunload handlers from the unloaders array:

function unload(cached) {
    ...
    if (cached.controllers) {
        forEach(cached.controllers, function (controller) {
            if (isFunction(controller.onunload)) {
                controller.onunload({preventDefault: noop})
                // Remove onunload handler from unloaders array
                for (var i = 0; i < unloaders.length; i++) {
                    if (unloaders[i].handler == controller.onunload) {
                        unloaders.splice(i, 1)
                        break
                    }
                }
            }
        })
    }
    ...
}

dead-claudia commented 8 years ago

If you already know how to fix it, feel free to send a pull request! It would be much appreciated.

On Thu, Nov 3, 2016, 06:34 Claudio Felber notifications@github.com wrote:

Description

For every controller that has already been unloaded, the onunload handler is called again when another component is mounted (e.g. when the route changes). Steps to Reproduce

In the subsequent example do the following:

Open the console

Click on settings link

Click on redraw a couple of times

Click on home link

You will see multiple UNLOAD messages in the console, one for every controller that has been created and already (!) been unloaded.

<!DOCTYPE html>

Note: The random noise has been added to force the controller to be recreated. Fix

Extend the unload() function in order to have it remove onunload handlers from the unloaders array:

function unload(cached) { ... if (cached.controllers) { forEach(cached.controllers, function (controller) { if (isFunction(controller.onunload)) { controller.onunload({preventDefault: noop}) // Remove onunload handler from unloaders array for (var i = 0; i < unloaders.length; i++) { if (unloaders[i].handler == controller.onunload) { unloaders.splice(i, 1) break } } } }) } ... }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/lhorie/mithril.js/issues/1382, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBJx-3xmeoQyupxvyF3yh1m0TW3mJks5q6bivgaJpZM4KoMyc .

barneycarroll commented 7 years ago

I wrote a simpler test case (with less noise ;P) here: https://jsbin.com/joyixep/edit?js,console,output

I tried to fix this in #1424 but couldn't because of what appears to be a bug in the v0.X test framework, Mithril itself, or an oversight on my part: in the test case, the subcomponent doesn't trigger onunload when it's removed directly. Any insight welcome.

dead-claudia commented 7 years ago

Closing - last v0.2 release has been cut.

MithrilJS / mithril.js

When the route changes onunload is triggered for controllers that have already been unloaded #1382

Description

Steps to Reproduce

Fix