How does unmounting a component affect other mounted components?

Update: tl;dr: I suggest adding an onunmount method and per-mount-root redraw strategy support to prevent surprises when mounting and unmounting components. A three-point summary of the first eleven comments:

There is a bug (or inconsistency) in Mithril 0.2.0 unmounting behavior relative to the documentation. The documentation suggests mounting a component does not affect other components except for a redraw. However, in Mithril 0.2.0, mounting any component causes all other mounted components to have their nested components re-initialized; the top components retain their state, except for the component being mounted or unmounted. This issue is demonstrated by a JSFiddle and an interactive test in a pull request (issue #695). To fix this inconsistency would probably require supporting a per-root redraw strategy (and it might be worthwhile to support top-level parametrized components when making such a change). Any affect of such a change on apps using m.route would have to be thought through though. Alternatively, it could be decided that mounting any component should indeed cause all other components to be re-built as a conceptual "route change" (although that might also require a code change, given only the nested subcomponents are rebuilt now). In either case, the documentation needs to be improved to be consistent with whatever is decided
The "onunload" method conflates two concerns: unloading a component and preventing unmounting. This leads to unexpected behavior where some components may unload when other components keep running (as documented in issue #552). Ideally, as one approach to separating these concerns, there should be a new method (perhaps called "onunmount" or "onunmountrequest") where components can call event.preventDefault() to prevent unmounting, like if they have unsaved changes (perhaps after confirming with the user). No unloading should be done in an onunmount method though because the unmount might not proceed -- unloading tear-down should only be done in "onunload". This change would break some existing code, as with such a change, onunload would (ideally) no longer have an event passed to it, and any existing onunload code calling preventDefault would need to be changed to put that code in "onumount". A call to event.preventDefault in onunload is ignored when unloading components due to changes in a view (as opposed to unmounting). So, this change would mean less surprises when writing an onunload method -- surprises which may otherwise require using "config context onunload" as a workaround to get correct unloading behavior. Adding this second method would probably also make it easier to implement a simpler fix for the unmounting behavior mentioned in the first point -- by making it easier to remove a global "unloaders" array and also by simplifying the current implementation which deletes and sometimes restores onunload methods from controllers. For consistency, it might make sense to support onunmount both in controllers and in context configs, since wrapped HTML widgets might also want to reject unmounting. There also may be a memory leak related to unloaders, which are currently added to a global array as controllers are created, but that array is only reset on a call to m.mount; such a leak would be fixed by these proposed changes.
There is an inconsistency in the meaning of the redraw "all" strategy. The top-level component retains its state during a redraw "all" (except for newly mounted or remounted components). It is only the state of nested components that is reset. I'm not sure whether it is worth changing anything about that, but it is another thing to consider when making these other changes.

=== Original first comment below:

I've been converting a ~25,000 line single-page Dojo/Dijit app to Mithril (and TypeScript) and so far it has been going very well; thanks so much for Mithril; it has been a real pleasure to work with. Here is the biggest stumbling block I've run into so far with the 0.2.0 release code. When I mounted and then unmounted a component on a div (to act as a dialog), a component in a different div which was previously mounted was recreated (losing its local controller state) during the resulting global redraw. I have a workaround (to mount all components at the start and leave them mounted), and the issue may well be in my code somehow. So, I'm mainly raising this in case it possibly is an issue with Mithril that might affect someone else. I was hoping someone might at least look at Mithril's use of the "unloaders" var and perhaps explain to me how just one array could be used to support unloading individual components?

== More details

I spent a couple hours trying to understand this behavior, but so far I have not been able to isolate this specific issue. One of my test cases is at the link below (based on the fiddle from Issue #674), but it works fine. In that test case, a button mounts another a component with a button on a second div, where the second button unmounts the second component. However, as that simplified code works fine, I wonder if it is possible that the issue I saw could be due to having a couple layers of nested components in the actual application? https://jsfiddle.net/pdfernhout/7yjks1ys/3/

I know this is not much to go on, and it is possible the issue is just in my code. Sorry I have not had time to isolate the issue further right now. I added keys everywhere just-in-case and made sure the mounted components were not parameterized (Issue #638), but the problem persisted. As a workaround, I rewrote the app's dialog code to just mount the dialog component once at startup and then leave it mounted. The dialog now just appears and disappears based on checking for some global content that defines its content, and there are now no issues with extra re-creations of other components in other divs. So, this issue is not a show-stopper for me; it is just a remaining curiosity.

The main reason I'm raising an issue now though, even without a reproducible case, is that when looking through the Mithril code, I noticed something unexpected as far as the design. Unless I've misunderstood something, there appears to be one global-to-Mithril "unloaders" array, defined at the end of a very long line (line 550, character 193 of the 0.2.0 release). That all looks suspicious to me. :-) I would expect that each mounted component would have a different "unloaders" array associated with the mount root somehow. Otherwise, how would Mithril know how to just unload components associated with the specific root when it is unmounted by setting it to null? While it might be possible to use just one array to store unload information for all mounted components, there does not seem to be any flag on objects in that array which might otherwise be used to filter for components associated with a mount root. So, I remain puzzled as to how it could work. Yet it apparently does (at least in the simple test case). Also, as an aside, I feel in this case the code might be easier to read if each of those vars was defined on its own line, as it would be easier to see at a glance where "unloaders" was defined.

Since the simple test case above works fine, there may just be something I am not yet understanding here. Could someone perhaps explain to me how the component unmounting & unloading process is supposed to work when there are multiple mounted components? Or perhaps someone who understands the vdom build process better than me at the moment could have a quick look at how the "unloaders" var is actually used to see if it seems plausible at all that unloading a mounted component might under some circumstances affect other mounted components (or their nested subcomponents) with onunload handlers?

This seems to be an actual bug in Mithril 0.2.0 mounting/unmounting. Here is an updated fiddle that shows the issue: https://jsfiddle.net/pdfernhout/sr5htexw/

I had to add an internal component to the first component in the first div. The internal component displays the time it was created and also has a text input you can edit. That component gets (incorrectly?) re-initialized as to both time and input when the second div is mounted or unmounted.

I wonder if there might also be another aspect of the issue related to why the simpler test case works? Perhaps the component at the first level of a mounted root is treated differently than internal components under it as far as unloading?

In looking at the Mithril code again, it seems that "unloaders" should probably be accessed as "unloaders[index]" (where "index" is used to map to the specific root). This would be similar to how "controllers[index]" is used. Making that change would provide a different set of unloaders for every root.

I'm working towards a fix. However, I've run into a snag. The "index" in the build function may not be the same "index" in m.mount, so the fix may not be quite as simple as in my previous comment.

I don't have a fix, but I have submitted a pull request for a test that shows the issue: https://github.com/lhorie/mithril.js/pull/695

==== More details

First, as background, if I understand this correctly (and I may not), it seems controllers for mounted components are being created at two different times. The first time is when the top-level component is mounted with m.mount, where only the top-level component's controller is created. The second time is when a previously created component is being rendered via calling view (recursively) and any nested components have their controllers created at that time.

In order for just the unloaders for a mounted root component and its nested components to be called when a root is unmounted, something needs to change. One option is to have one unloaders array for each mounted component (which can be iterated over). Another option is that the unloader functions could to be called via some recursive function moving down through nested controllers. However, since controllers can come and go in theory with each call to view, that adds another twist to this issue, that unloaders should be called when the build process discards a vdom node with a controller.

One (strawman?) way forward that looked fairly simple at first might be to have another global like "unloadersForRoot". This could be set to an empty array at the start of m.mount, used in the build method, and then the resulting contents could be put into unloaders[index] in m.mount. This array could be setup again when each root was being rendered. However, it would seem would just accumulate until the top-level component was unmounted, since who would call them otherwise? So, this approach, if it could work at all, might not unload components as soon as you would expect (which would be presumably when they were dropped from a view).

Also, a comment in m.mount makes me wonder if that strawman approach would be adequate for other reasons. The comment reads: "//controllers may call m.mount recursively (via m.route redirects, for example)" . If m.mount is called recursively, then it would seem some previously loaded components from m.build would need to be unloaded before the entire m.mount function was done running? So, again, a global array that was set and reset as each root was worked on might not be adequate because of this internal recursion.

In m.build, there is a "controllers" and "views" array made for each OBJECT. The "controllers" (if any) are stored in "cache". I wonder if ultimately unloaders may need to have something similar done? Or perhaps unloading should just be done recursively through nested controllers? Somehow it feels to me that recursing through the unloaders is ultimatley the right thing to do, sort of as the inverse of the build process.

Anyway, I'm not sure how to proceed further on this without first obtaining a much better understanding of the vdom process and/or setting up more test cases. I may not have time for that right now. But at least with the test case, others developers can look into this as well and hopefully come up with an elegant approach in keeping with the rest of Mithril.

As another step towards a resolution, using the new test, I tried modifying Mithril to have one unloaders array per mount point to see what happened. The results did not turn out quite the way I expected.

The change involved passing a "rootIndex" as the first argument to all the build functions. The var rootIndex was calculated in m.render using "var rootIndex = roots.indexOf(root)". To have one unloaders array per mount point, I also used references to unloaders[rootIndex][i] in m.mount (renaming those index references in m.mount to rootIndex for consistency) and setting unloaders[rootIndex] to an empty array if the root was being added.

I knew this approach probably would not solve the entire issue (like components being added or removed in views and unloaders piling up from that, or recursive calls to m.mount), but I thought it might at least make things a little better than they are now. This change did fix the issue of the unloaders begin called incorrectly for the first mounted component's internal component. However, the internal component is still being still recreated -- just without being unloaded first. (Update: it turns out the internal component is still being unloaded, just from the build function, not the mount function. This happens even after adding a key to the internal component.) So, the internal controller is still losing its state when the second component is mounted or unmounted. Obviously, there is some other piece of the puzzle I'm missing here, since I thought this approach would work within some limitations. As mentioned in a later comment -- that puzzle piece is a redraw "all" strategy set by m.mount.

I put that experimental change in an "unloading_experiments" branch here: https://github.com/pdfernhout/mithril.js/tree/unloading_experiments

This is the specific commit: https://github.com/pdfernhout/mithril.js/commit/662261bedd11c7a18d28f22f8f1cce16710fb3e8

Again, this change does not resolve the issue. It is just an experiment to think about.

Summary: I'm now wondering now if this behavior might be more a feature than a bug, but that there might still need to be related code changes to make everything consistent either way?

=== More details

As I continue to look into why that component was rebuilt even in the modified version of Mithril, I see that the redraw strategy is set to "all" in m.mount.

if (!isPrevented) {
    m.redraw.strategy("all");
    m.startComputation();

This flag is checked in this section of code in the build method for OBJECT:

var controllerIndex = m.redraw.strategy() == "diff" && cached.views ? cached.views.indexOf(view) : -1;
var controller = controllerIndex > -1 ? cached.controllers[controllerIndex] : new (data.controller || noop);

So, it seems to me that this will cause all existing nested controllers underneath any top controllers to be discarded and recreated in all mounted components when any individual component is mounted or unmounted? The top level controllers are not rebuilt though, because redraw always just uses the top level controllers previously created by m.mount, which are never reset (except if they were to be remounted). So, top-level controllers and nested controllers are indeed treated differently during a mount operation.

In looking further at the code, and from very limited testing, it seems onunload functions for components are called individually on removed components during build, even for nested components (although I'd suggest more testing of that). So, if that part works, I'm wondering if the global "unloaders" array is even needed at all? Perhaps removing that global unloaders array entirely and handling the redraw strategy flag differently somehow might be a possible solution?

However, it is not clear to me if the redraw strategy flag could be easily set one way for redrawing existing components and another way for drawing a newly mounted component? That issue may slide into other issues raised about having different mounted components redraw differently.

In the documentation for m.redraw, it says:

When the flag is set to "all", Mithril throws away the current view and redraws from scratch. This is the default for going from one route to another."

While I would not consider mounting and unmounting a dialog to be a route change, one might argue this is a design "feature" instead of a bug, treating any sort of mounting or unmounting as a "route change". At the very least though, this behavior probably should be documented on that m.redraw page and also in the page for m.mount.

If a redraw strategy of "all" is indeed the expected behavior, then having a single global unloaders array might be OK? Although it still seems like the unloaders array would leak memory unless items are removed from it when subcomponents are unloaded -- which does not seem to be the case? And also I'd guess that unloaders might even be called twice in some situations (if nested components were deleted and then a top-level component was unmounted)? And also the behavior is inconsistent since the top-level component is not re-initialized while all nested components are. So, whichever way it should be, there may need to be some code changes for consistency.

So, I'll wait to see what others have to say on whether this issue as a bug or a feature and whether it needs further work and how. If this indeed is a "feature", then the text in the unloadingTest.html file in the pull request would need to be changed to reflect that fact about expected behavior.

Again though, a workaround for all this is just to mount everything at startup and then not unmount or remount anything afterwards. Or alternatively, if you keep all your GUI state in the top-level mounted component (or a model outside the components entirely), and reference that state from nested components, you probably won't notice the nested components getting recreated. In my case, I had a nested component (a table with a detail panel) that maintained internal GUI state (on whether the detail panel was open or not), and that state was being lost when a dialog popped up until I implemented a workaround.

In regards to entirely getting rid for the unloaders array being used in m.mount, here are some more design issues to consider. Ultimately, and I'm sorry to suggest this given the API implications, but providing consistent behavior for unloading components may require a breaking API change, by adding support for a "preunmount" method (or similar) to controllers where "event.preventDefault()" can be called and then ignoring the use of preventDefault in the onunload methods (probably by just not passing an event into onunload for controllers). But, perhaps someone else can think of a non-breaking way to make this all consistent? In general, Mithril is a joy to work with, and I'm glad Mithril has recently added support for components, and these issues can be worked around like by avoiding unmounting anything or keeping all GUI state in the top level controller with 0.2.0 -- so, I'm just trying to help make a great library even better.

==== More details

This is the main related code using unloader handlers from Mithril 0.2.0 (although remember onunload can be called elsewhere in the code too):

for (var i = 0, unloader; unloader = unloaders[i]; i++) {
    unloader.handler.call(unloader.controller, event)
    unloader.controller.onunload = null
}
if (isPrevented) {
    for (var i = 0, unloader; unloader = unloaders[i]; i++) unloader.controller.onunload = unloader.handler
}
else unloaders = []

For reference, an unloader handler is defined else where in the build function like so:

if (controller.onunload) unloaders.push({controller: controller, handler: controller.onunload})

The m.mount code iterates through the unloaders stored in the handlers, calling each. Afterwards, it checks if any of them want to prevent unloading (if one or more has set the isPrevented flag), and if so, it puts back all the unloaders. If there was no unloaders array, that would have to be some sort of recursion to iterate through the unloaders.

The approach of deleting an unloaders from a controller and putting it back might have to change somehow without an array of special unloader handlers. In general, it seems to me that deleting a method from a controller (and maybe putting it back later) to signal something just feels problematical as far as being harder to understand and debug ("where did the method go?"), even if it may work. If the controllers were recursed through, there would either need to be some flag, some shadow copy of the onunload method, or there would need to be a separate method to use to check if it was OK to unload (see below for more on that last option).

One possible issue here though is conceptual, and may need to become its own separate issue. What are the expectations for an "onunload" method? This code seems to imply that an onunload method may be called multiple times. If so, it would need to have guard clauses for actions it should not do twice.

For example, consider if you have one nested component that blinks every second via a timer (perhaps as it polls a server for its status), and another nested component with an input field that needs to be validated. Assume these two are subcomponents of a component mounted on one div. When you go to mount or unmount something on a different div, these two subcomponents are requested to be unloaded. The first dutifully shuts down its timer. The second rejects the unload by calling "event.preventDefault()". The unloaders are then put back into the controllers, but the blinking light component is now essentially broken because its timer has been shutdown. Later, when the component is really unmounted, the blinking light component will presumably have unload called again, and might try to shutdown the same time a second time (depending on how it is written).

This approach to unloading (or rejecting unloading) may work if there is only one top-level component with no nested subcomponents. However, it seems to me it would not work reliably if there are multiple nested components which might disagree about rejecting the unload. It is possible that the only reason unloading usually seems to work OK with this approach is the "all" strategy used for redrawing, which will re-initialize nested subcomponents, and so, in our example, start up the timer again as a side-effect of the re-initialization. However, the top-level component won't be re-initialized since it is handled differently, and so I'm guessing it would remain unloaded, while its subcomponents are then working OK, but with reinitialized state? It would be good to have a test case for that top-level unloading behavior to be sure. Update: it turns out components are not redrawn if one nested component in any mounted component rejects the unmount and so they all won't be reinitialzed; so components can indeed end up in an inconsistent state if controller.onunload is used as opposed to "config context onunload", as discussed in issue #552.

The semantics of "preventDefault" work somewhat differently for HTML nodes and events (although, with legacy JavaScript/DOM use there remains confusion, including perhaps on my part). For an HTML node event, when you call preventDefault, you in theory cancel the event (if it is cancellable). Returning false from an event handle also may prevent an event from bubbling up entirely. So, the "main" action may not happen (depending on what it is, there being historical inconsistencies in the DOM, adding to the confusion). Semantically, the equivalent of supporting "preventDefault" might be if Mithril components had a "preunload" or "beforeUnload" or "isOKTOUnload" method where the unload event could be rejected at that point before onunload was called. So, using "preventDefault" in the current way in Mithril onunload methods could lead to some application developer confusion, or in this case, inconsistent internal behavior. That is because, you can't prevent the default unload action if other subcomponents have already unloaded. In the Mithril case "preventDefault" for onunload means more like, "prevent it for me, and try to undo unloading the other components". Maybe in theory that could be made to work, but it would require being able to consistently and completely roll back any state changes from other unloaders, which would be a much more complex undertaking than just putting back a removed onunload method on a controller.

Another aspect of this inconsistency is what "preventDefault" means when a component is unloaded because it is being removed during the redraw/build phase. A developer might expect that calling event.preventDefault if you have unsaved input would actually do something every time. However, if a component goes away during a build, then preventDefault would be ignored. This could lead to a component leaving timers around or such. Although, looking at the 0.2.0 code, apparently sometimes no event is passed to onunload, so I'm guessing in these cases at least there would probably be come sort of exception in the onunload method when it tried to reference preventDefault on an undefined event.

From the documentation on m.component:

If a component's controller contains the function onunload, it will be called under one of these circumstances:
    when a new call to m.mount updates the root DOM element of the component in question
    when a route changes (if you are using m.route)

Later, the m.component documentation also later says:

Components that are nested inside other components can also call onunload and its e.preventDefault() like top-level components. The onunload event is called if an instantiated component is removed from a virtual element tree via a redraw.

That first cited part of the documentation disagrees with the current behavior, where all mounted components have their nested subcomponents rebuilt on any component being mounted/remounted. The second cited part agrees with the behavior I have seen as far as redrawing/build. It might make the documentation clearer to include some of that information in the second cited part into the list in the first cited part (so the list of circumstances for calling onunload would have three item).

Further, with the current code, unloaders in other mounted components could cause a different mounted component to have its unload rejected. So, in the case I was working on with a modal dialog (before I rewrote it), it seems like potentially an unsaved edit on the main page could, say, cause the unmount of a warning dialog to be rejected, leaving the dialog mounted and unclosable?

So, is the documentation correct there (unmounting happens individually for the "component in question")? Or is the code correct (unmounting sorts of works like a total routing change for all mounted components)?

In either case, it would seem the current unloading strategy may need to be rethought. Perhaps controllers that want to reject unloading could have a separate method to indicate that? Although that would break the current unloading API. Or, the event passed to the unloader could have some sort of stage marker in it, a polling stage ("is it ok with you to unload, maybe we won't?") and a committed stage ("really unload now regardless of your preferences"). But that approach, where the event had different meanings, might be even more confusing, so just adding a "preunload" (or "preunmount") method might be best. In the previous example with the blinking light and the unsaved edit, with such an API change, the blinking light would never have its onunload method called if the unsaved edit component has called event.preventDefault() in its preunload method. On further reflection, "preunload" might not be the best name for that function. If the function is only called before unmounting, and it is not called when a component is discarded during the redraw build process, then "preunmount" might actually be the best name for it, since, in a way, that new function would actually have nothing to do with unloading itself, but is more about unmounting and whether that is OK.

To make this transition to an API change by adding support for "preunload", it of course might be possible for a time preventDefault could still be supported in a deprecated legacy way, leaving around the current approach while layering a new one. However, given components are new and Mithril is only at 0.2.0, and given supporting the old way would mean leaving in cluttery code that probably still worked inconsistently, I'd suggest considering just not passing an event to onunload after such an API change as a breaking change. Then any application code that uses preventDefault in an onunload method would probably immediately fail during testing (due to a reference to an undefined event.preventDefault field) and could be fixed right away by moving that test and preventDefault call to a preunload method in the controller.

Also, for a more general design consideration, below are all the references to onunload in the 0.2.0 code. There are a lot of them. With 20/20 hindsight, having so many references seems to me to just be asking for some sort of inconsistency. Perhaps there could be come way to consolidate the references to onunload to just a few instead of twenty five (on sixteen lines), including by somehow handling the top level component somehow the same as its nested subcomponents? Then there would be less chance of an inconsistent use of onunload, like passing an event to it sometimes but not others as is the case now. [See update below on my confusion on not realizing there are two different types of onunload functions for controllers and config contexts with different signatures.] Moving to a recursive approach by calling preunload and then onunload (as the inverse of build) might require less lines of code "spent" (E.W. Dijkstra) to achieve full consistent functionality, in keeping with Mithril's elegant design. While a recursive approach to seeing if components can be unloaded or actually unloading them might be more elegant and more consistent, there might be a minor performance penalty as a tradeoff though, from recursing through nested controllers (or intervening components) instead of iterating through a short list of "unloaders". Update: after writing this last paragraph, I realized I was in it confusing the "onunload" function for a controller with the "onunload" function of a context returned to a config function when I made this list of references on onunload. So, there is not as much code duplication here as I first thought. When onunload is called without an event actually happens with config contexts, not controllers, and so is correct as-is. I'm leaving this paragraph in place just so others can learn from my own confusion here about two types of onunload functions. However, the approach mentioned above would have the benefit of making the signatures of the two onunload functions consistent, with both not taking any arguments. To be consistent, contexts perhaps should support a "preunmount" method too.

mithril.js (25 matches [to "onunload"])
254: if (controller.onunload) unloaders.push({controller: controller, handler: controller.onunload})  
267: if (cached.configContext && typeof cached.configContext.onunload === FUNCTION) cached.configContext.onunload()  
270: if (typeof controller.onunload === FUNCTION) controller.onunload({preventDefault: noop})  
297: if (controller.onunload && controller.onunload.$old) controller.onunload = controller.onunload.$old  
298: if (pendingRequests && controller.onunload) {  
299: var onunload = controller.onunload  
300: controller.onunload = noop  
301: controller.onunload.$old = onunload  
441: if (cached.configContext && typeof cached.configContext.onunload === FUNCTION) {  
442: cached.configContext.onunload();  
443: cached.configContext.onunload = null  
447: if (typeof controller.onunload === FUNCTION) controller.onunload({preventDefault: noop});  
580: unloader.controller.onunload = null
583: for (var i = 0, unloader; unloader = unloaders[i]; i++) unloader.controller.onunload = unloader.handler
587: if (controllers[index] && typeof controllers[index].onunload === FUNCTION) {
588: controllers[index].onunload(event)

In thinking some more about that the name of the new method would be, here are a few options other than "preunmount(event)". It could be called something like isUnmountable(), mayUnmount(), allowsUnmounting(), getUnmoutableStatus(), or something like that. Alternatively, the status function could also be named semantically from the perspective of component state, rather than the underlying concept of unmounting. So, the function name instead could instead be "isDirty" or "hasUnsavedChanges" or perhaps something like that. Still, there may be other reasons a component might not want to allow unmounting (although I can't think of one at the moment), so perhaps something like "isUnmountable()" is more general.

Since the method is just to determine if the component is allowing itself to be unmounted, it probably could just return a boolean instead of taking an event on which preventDefault would be called. Such a change to just return a boolean could then simplify the implementation of such methods as well as the library code that called them. However, with a boolean being returned, there would always be the risk of someone implementing such a method and forgetting to return a result and so preventing unmounting the top-level component.

Issue #552 is related to this one. The author, tobscure, writes: "When I route away from that root component, firstly the sub-component unloaders are called (and thus my scroll event handler is turned off), and then the root component's unloader is called, cancelling the transition. But it's too late - my event handler has already been lost."

The suggestion by Leo there was to use "config context onunload" instead of controller unload and instead of tobscure's suggestion of adding a destroy hook to complement the config hook.

It is true that using "config context onunload" is a workaround for an aspect of this issue (multiple calls to onunload and unintended unloading sometimes) -- even as it does not address the loss of controller state in other mounted components than the one being mounted or what may be a memory leak if unloaders pile up. However, the situation and workaround would also imply that the meaning of the current "onunload" method is really more like "isReadyToUnmount".

Looked at it that way, developers are just usually getting away with putting unloading code in the (defacto) "isReadyToUnmount" method (as opposed to "config context reload") if they understand their application well enough to be sure an unmount will never be rejected via event.preventDefault being called by some unloader somewhere (even in another mounted component tree over on the other side of the page). For reliability though, this implies any real unloading (like to shut down timers and such) in any component intended to be generally reuseable should be done via "config context unload" to avoid the issue tobscure experienced.

That just seems a confusing situation to me as regards to the meaning of that API for "onunload" given its name. Especially with the new needs created by supporting components in Mithril, the root cause of the situation to my mind is trying to get one method (onunload) to do the work of two methods: "isUnmountingOKByYouOrDoYouWantToVetoIt()" and "reallyUnloadNowIMeanItThisTime()". :-)

On more reflection for the api name for a new function, I can see a potential benefit to using "onpreunmount" or, perhaps, "onunmountrequest", with an event object passed in, where the method calls event.preventDefault if the unmount should be rejected. While that approach would take more work to implement for both an app developer and a library maintainer, I'd expect it would help with a common use case of having a form with several components that may have unsaved data. As an example, on receiving an onunmountrequest event, each component could perhaps put up an alert to ask if if was OK to discard the changes. But that might mean several alerts, one for each component. Passing in an event object to onpreunmount provides a place for application components to store information about the result of the first alert and communicate with each other. Still, an alternative to that is to put the onpreunmount methods higher up the controller hierarchy, perhaps at a form level, where one method could examine the state of the entire form, and then return a boolean about whether unmounting was OK. It's hard to guess about how component authors might want to coordinate reuseable components that require saving state, so I don't know what's best there. The simplest approach is to just have methods that return a boolean, without even passing in an event object. But passing an event object around does provide potential future flexibility at perhaps fairly little cost. Still, a new API for onunmountrequest could start with not passing in an event, and if there was a demand, an event could be added later as an argument.

Another variation is that an onumountrequest method could, if desired, return a promise instead of a boolean (or perhaps instead pass a promise to event.preventDefault). Then m.mount could wait on all those promises before then doing the unmount/remount. So, a bunch of unsaved widgets could then all return promises related to some single confirmation dialog or network request or such. However, that might just be too much complexity for too little benefit. The alternative is to simply rejecting the unmount (perhaps with a window.alert) instead of, say, trying to save data over the network and then let the unmount proceed, or trying to run some complex confirmation GUI while the unmount was pending rather than just use a simple window.confirm. Again, a new API for onunmountrequest could always start with the approach of returning booleans, and if it was not good enough, implement promise support.

In continuing to think on this, while I like a more descriptive name like "onunmountrequest", perhaps that addiitonal method should just be called "onumount", in keeping with other shorter Mithril names? The docs would need to make clear the difference between onunmount(event) and onunload(), which will certainly be a bit confusing initially. Essentially, onunload() means unload now, and onumount(event) means an unmount is going to happen unless someone calls preventDefault on the event.

Also, I want to re-emphasize that adding a second method to a controller is not enough to resolve this issue. There is still the issue of a global redraw "all" strategy being set in m.mount. Probably another change would be needed to support per-root redraw strategies. That could be implemented by storing an object with a root, a redraw strategy, a top level vdom component, a controller, and a (ideally parameterized) controller constructor in the roots array -- instead of having a roots array with a root element and a controllers array with a top level controller and a components array, all with a common index. This root object could be passed as the first argument to the build function. The root object for a root element would be found by iteration. Ideally these objects would be removed from the array when a root component was unmounted. The m.redraw.strategy function could take a second optional argument which would be the root the strategy applied to, otherwise the strategy would apply to all roots. In m.mount, the strategy would first be set to "diff" for all roots and then "all" for the root being mounted. As a caveat, there might be expectations about the m.route function which may interact with any changes to the redraw strategy in m.mount. So, this may need further review to ensure such a change does not break m.route unexpectedly in practice.

There is another related issue as well. Currently, a redraw strategy of "all" will leave the top-level component's controller as-is, but will re-initialize nested component's controllers. I would expect some Mithril apps now depend on that behavior, treating the top-level component like an application that stores state. Also, m.route calls m.mount, and so apps that use m.route may also expect this behavior of top-level components retaining their state (except the one specified in m.route's mount point) while nested components are reset. However, it seems to me that logically a redraw "all" strategy should recreate even the top-level controller, resetting the entire GUI to a state based on the application's model, rather than retain state from the top-level controller. However, I don't know such a change is worth making because it will likely break existing apps, even though, conceptually, it seems to make a sort of sense to me. A related aspect is that top-level controllers are not supposed to be parameterized (mentioned in issue #638) -- but it seems to me like you should ideally, for consistency, be able to do that. Having parameterized top-level components might make it more acceptable for a redraw "all" strategy to rebuild everything. The only time in Mithril 0.2.0 that a redraw strategy is set to "all" by the library is when mounting a component in m.mount, which means the top-level controller is being initialized anyway. If there were per-root redraw strategies to deal with separating components, then other mounted controllers would not be affected by this strategy on a mount. So this redraw "all" issue would seem probably to be only about the expectations of application code setting that flag -- again, with the caveat about m.route calling m.mount. If major changes are being made to how root objects are handled, it might be good to keep this issue in mind.

I'm going to update the first comment with a summary of all these issues from these eleven comments:

There is a bug (or inconsistency) in Mithril unmounting behavior relative to the documentation. The documentation suggests mounting a component does not affect other components except for a redraw, but in practice mounting any component causes all other mounted components to have their nested components be re-initialized. This issue is demonstrated by a JSFiddle and an interactive test in a pull request (issue #695). To fix this might require supporting a per-root redraw strategy, and perhaps top-level parametrized components. Any affect on apps using m.route would have to be thought through though. Alternatively, it could be decided that mounting any component should indeed cause all other components to be re-built as a conceptual"route change" (although that might also require a code change, given only the nested subcomponents are rebuilt now). In either case, the documentation needs to be improved to be consistent with whatever is decided
The "onunload" method conflates two concerns: unloading a component and preventing unmounting. This leads to unexpected behavior where some components may unload when other components keep running (as documented in issue #552). Ideally there should be a new method (perhaps called "onunmount") where components can call event.preventDefault() to prevent unmounting, like if they have unsaved changes (perhaps after confirming with the user). No unloading should be done in an onunmount method though after such a change. This change would break some existing code. With such a change, onunload would no longer have an event passed to it, and any code calling preventDefault would need to be changed to put that code in "onumount". Adding this second method would probably also make it easier to implement a simpler fix for the unmounting behavior mentioned in the first point -- making it easier to remove a global "unloaders" array and also the current implementation that deleted and sometimes restores onunload methods from controllers. There also may be a memory leak related to unloaders, which are currently added to a global array as controllers are created, but that array is only reset on a call to m.mount; such a leak would be fixed by these proposed changes.
There is an inconsistency in the meaning of the redraw "all" strategy. The top-level component retains its state during a redraw "all" (except for newly mounted or remounted components). It is only the state of nested components that is reset. I'm not sure whether it is worth changing anything about that, but it is another thing to consider when making these other changes.

@pdfernhout , hi! Have you tried to move internal component state out of controller to dedicated structure, that is into VM in terms of MVVM? I guess it would be in accordance with those articles Leo has supplied us so generously :)

I mean something like this one:

        var vm = {
            text: m.prop("initial"),
            created: new Date().toISOString()
        };

        var InternalComponent = {
            controller: function(args) {
                console.log("+ executing controller for internal");
                return {
                    vm: vm,
                    onunload: function() {
                        console.log("- onunload() in controller for internal");
                    }
                };
            },
            view: function(ctrl, args) {
                console.log("^ executing view for internal");
                return m("div", [
                    ctrl.vm.created,
                    m("br"),
                    m("input", {
                        value: ctrl.vm.text(),
                        onchange: function(event) {
                            ctrl.vm.text(event.target.value);
                        }
                    })
                ]);
            }
        }

@gaydenko Thanks for the reply. Yes, I agree, moving the state of nested components up to the top-level controller like in your example would be a workaround for the issue. Top-level components are not reinitialized in Mithril 0.2.0 during a mount (except for the specific one being mounted or unmounted).

While applications could be designed like you or Leo outline, I feel that does limit the value of components because they can't be considered independent systems as far as storing their own local GUI-related state. Examples might be whether a component is open or closed (as in a menu or form) or what display mode they are in (like an analog or digital clock) or perhaps what specific item is selected in a list. It is harder to build arbitrary recursive assemblages of components if you always have to think about how to keep all that state at the top level (although it is no doubt possible). This just seems like an arbitrary limit to me based on the current implementation of unloading and global redraw strategy. To me, having components that seem to get arbitrarily reset as other parts of the page change increases the "cognitive" load of using the Mithril library, because you need to learn non-obvious limitations that violate (at least my own) expectations (as well as disagree with the current documentation).

Still, it's always possible to decide the mounting aspect of this issue indeed is a feature and not a bug and update the documentation to reflect that. I'm open to arguments in that direction. Maybe it is best for whatever reasons to say all GUI state will be kept in a top-level component and design components that work within that limitation? It also may not be a priority to change the mounting behavior even if it was seen as important by some. In general, the only time this issue will come up is when you mount multiple components and then remount one -- and even then, it will only affect the mounted components that have nested subcomponents and which are not written in the way you outlined. The likely (small) unloaders memory leak and the onunload preventDefault issues (which also has a workaround via using config) could be considered separately in that case.

@pdfernhout , it was just an example with minimum changes. In real code we can wrap a component state inside the component with IIFE:

        var InternalComponent = (function() {
            var api = {};

            var vm = {
                text: m.prop("initial"),
                created: new Date().toISOString()
            };

            api.controller = function(args) {
                console.log("+ executing controller for internal");
                return {
                    onunload: function() {
                        console.log("- onunload() in controller for internal");
                    }
                };
            };

            api.view = function(ctrl, args) {
                console.log("^ executing view for internal");
                return m("div", [
                    vm.created,
                    m("br"),
                    m("input", {
                        value: vm.text(),
                        onchange: function(event) {
                            vm.text(event.target.value);
                        }
                    })
                ]);
            };

            return api;
        })();

@gaydenko Thanks for the great example of using an IIFE (immediately-invoked function expression) to make a component constructor function passed into m.component. I learned something from it. Here is a modified JSFiddle with your code showing internal component state preserved through mounting an unmounting: http://jsfiddle.net/pdfernhout/sr5htexw/1/

Essentially, you are turning the component into a (global) function that returns the component and binds/stores the initial state. It definitely improves things. Thanks! However there are at least a couple limitations I can see.

First, what happens if I change the InternalComponent's editing area from a text input to a textarea? And then manually resize the textarea to be bigger than the default and add several lines text and scroll to the end? And then "open" the second component? The textarea's state as to size and scroll position is still lost as the InternalComponent gets rebuilt on each mount/unmount. Such rebuilding is also just excess computation for no obvious benefit I can see. Here is another change to the JSFiddle that shows that issue: http://jsfiddle.net/pdfernhout/sr5htexw/2/

Second, what happens if I want to have two InternalComponents with different state? They might indicate the time they were created, or have different internal stored text, or different open/close state, or might do different network requests. The current approach makes one global constructor function that has already defined its state once.

I can guess there might be at least one workaround to the second approach. As an example (not having tried it) perhaps a workaround could be maintaining a global list of instances of the component, where each instance is constructed with a consistent key passed in by args, and then the component looks up its state from the global object via that key. That global object could perhaps be maintained by the application and also passed in via args. That all might work, although it adds another non-obvious layer of complexity to writing components. Also, it might be a source of memory leaks, because when do you remove the data for each component? I guess you could use "config context unload" for correct unloading, but again this is adding another layer of complexity.

Or maybe, following your lead, there is some better way to create multiple internal components with yet another layer of IIFEs (stored in a parent component's controller) instead of global state of some sort? I'm not sure about that though, with the issue of recursively nesting components. Maybe it could work though, building further on your suggestion recursively -- with such constructor functions nested all the way up to the top-level component? But isn't maintaining such a tree of nested specialized constructors essentially just a different (maybe better) way of maintaining global state for components? Why not just keep the component state in the component itself and not throw it away when you don't have to (on a mount of another root)?

Because top-level components aren't parameterizable (apparently according to issue #638, but that may change), there is another related difficulty. What if you want to mount two of the same component on a page, just with different state? However, a workaround for that might be setting a flag, like the mount point div's id, and using that to look up component state in another global. Or, perhaps again building on your example, one could create parameterizable constructor functions for the constructor functions. Here is some related humor about "hammer factory factory factories...": Why I hate [overly abstract] Frameworks -- to be taken with a grain of salt though because indirection and wrapping can be useful ideas, and Mithril is a good example of those ideas used well to limit complexity. But again, making constructor constructor ... functions seems like extra conceptual overhead just to use components. Why can some components be parameterizable, but not the top-level ones? That is mostly a separate issue than the mounting issue and loss of local state due to a global redraw strategy of "all" after mounting -- but it interacts with it, to add complexity pushing one towards more global state (and global state is what I used to resolve the dialog issue that lead me to post this issue originally) -- or maybe alternatively more layers of indirection via IIFEs.

Global state can work well in smaller projects; but it generally does not scale well. Still, too much local state can be its own sort of problem as well. Too much local state is often a problem of many OO systems with many small objects vs. more procedural or functional systems that operate on bigger data structures. Although functional systems can also hide state in lots of small functions. In this case, debugging through layers of constructor functions is just going to be harder than only having one layer. Real working systems tend to have an appropriate mix of global and local state (or meshwork, hierarchy, and interfaces). So what is the appropriate mix here of global and local?

One of Mithril's big benefits is reducing the amount of abstraction in dealing with the DOM and components (compared to, say, Angular or Dojo/Dijit). Making IIFEs which access global objects just seems like an extra layer of abstraction when the alternative is that the components just work the way the documentation suggest they should. As has been said (paraphrasing Kevlin Henney), every problem in computing can be solved by adding another layer of abstraction -- except for the problem of too many layers of abstraction. :-) https://en.wikipedia.org/wiki/Abstraction_layer

However, I don't yet see a good workaround to the first issue on losing a textarea's sizing or scrolling information. I guess one could, in this case, track the textarea's size and scroll position (and perhaps selection or other properties) -- either with callbacks or on a timer. So, even for that, maybe a workaround would be doable; I'm not sure. Again though, that would be adding another level of complexity just to put a textarea up on the page (inside a nested component). That seems to me to be going in the opposite direction of Mithril's design principles.

Still, I have to admit, you have pointed me at a powerful idea for workarounds for building reusable components with Mithril 0.2.0. Thanks for adding another tool to my Mithril/JavaScript toolbox. However, I still see it as a workaround for unneeded rebuilding of component state. Just because we have power to do something, that does not mean we should actually do it, given alternatives (including doing nothing). And another alternative here which I feel is a better one overall is to make mounting/unmounting work like the documentation for m.component suggests: "Unloading components -- If a component's controller contains the function onunload, it will be called under one of these circumstances: when a new call to m.mount updates the root DOM element of the component in question ..." [my emphasis]

Thanks for explaining this in such much detail.

I recently bumped into this too on a page with multiple modules. Mounting a module clears out the textarea and input of other modules because of this.

Are there plans to fix this?

I can tell you all I've merged #695, which tests for this. It will need implementation first, and will require several changes under the hood (e.g. support for multiple roots).

Closing - last v0.2 release has been cut.

MithrilJS / mithril.js

How does unmounting a component affect other mounted components? #694