unload only if not mounted

[lukeburns]

currently on-load emits unload if a node is removed from dom. this pr requires that the node also not be mounted on the dom.

https://github.com/shama/on-load/issues/25 describes one situation in which this is helpful. this change is also helpful for managing nested elements, each with their own load/unload handlers. if an element is re-rendered, child elements only emit unload if no longer contained by the dom.

[bcomnes]

We can consider this along side https://github.com/choojs/nanocomponent/pull/75 but I think we need to characterize the actual desired behavior, and limitations of moving dom nodes around the page.

[lukeburns]

@bcomnes, can you say a little bit about how this affects the nanocomponent issue?

[bcomnes]

Nanocomponent debounces some on-load events because of how nanomorph and on-load interact https://github.com/choojs/nanocomponent/blob/master/index.js#L133-L146

It looks like maybe this is a similar solution? I need to dig in more, but my point is that, on-load should probably have a concept of dom node re-ordering built in and this could possibly do it, but we need to be carful and have a full understanding of the implications of the change and what we are trying to achieve.

[lukeburns]

i needed this pr to avoid multiple load/unload events for cached elements removed and immediately reinserted into the DOM as well. if you're wanting to move elements around without emitting load/unload events, this probably fixes it.

i imagine there could be scenarios with race conditions where an element is reinserted right after an unload event is emitted (say a large tree of components is re-rendered, and it takes a while for a cached child component to be reinserted in the page). this is not solved by this pr, but at the the very least i think it's reasonable to expect an element not to emit an unload event by on-load if it is still in the dom and its on-load id is the same.

there are cases where this might cause unexpected behavior. for instance, suppose nanomorph mutates an element to what the user expects to be considered a new element, but it does so without replacing the element. this would not trigger unload/load events even if the user expects it to. this user has two options if this pr is merged: (1) force nanomorph into replacing the node with a new one or better yet (2) simply make sure to wrap the element with another on-load handler, which they're probably already doing.

[lukeburns]

@shama, do you anticipate this causing complications?

[bcomnes]

@lukeburns sorry i never got around to looking at this this weekend due to traveling. What do you think about doing a beta release of a change like this to play around with, since its going to be a major verion bump either way.

[lukeburns]

all good -- that sounds good to me!

[lukeburns]

@bcomnes, if committing to this in a beta, we might commit to the same for onload behavior. i've already encountered a situation where this is helpful. could imagine it also being helpful with debouncing of load events that nanocomponent is dealing with.

this additional change may require changes to tests, because an element removed immediately after adding no longer emits a load event if it is not mounted when the mutation event is being handled. tests were failing for me locally (surprised by travis ci) — so i included changes to two tests as well.

[bcomnes]

By beta, I mean do some npm beta releases (4.0.0-beta1...) so people can easily play around with it and you are not blocked to the direction you want to take it. My primary open question on this are:

a) Are there consumers who depend on the non-debounced behavior? b) Will removing non-debounced behavior all together prevent some consumers from doing what they are doing now? c) Should the debouncing behavior be implemented as an option?

[lukeburns]

Just checking in on this. Any thoughts? I'm currently relying on a fork for https://github.com/lukeburns/morphable.

[bcomnes]

Since nobody else chimed in, we should try to get this into a new major version

[lukeburns]

👍

[lukeburns]

Any steps to take before that?

[bcomnes]

No, I can do it right now. Apologies, been traveling for the last week.

[lukeburns]

all good. thanks!

[bcomnes]

@lukeburns how does this work for nodes inside of shadow dom or iframe documents? Is that a use-case we need to cover?

[bcomnes]

🎉on-load@4.0.0🎉

[bcomnes]

@lukeburns would love your thoughts on the remaining PRs that are open. I've not had my head in this codebase for a while.

[lukeburns]

hooray!!

this won't work for shadow/iframe nodes as is.

it checks to see if the element is contained by the root element of the document before triggering load/unload events, and shadow/iframe nodes will not be detected as in the document (shadow node example: https://jsfiddle.net/39mo162v/15/, iframe node example: https://jsfiddle.net/9xuxntpL/932/).

we could iterate through shadow roots / iframes to check for containment, if this is an important use case.

[lukeburns]

@bcomnes thoughts on shadow and iframes nodes?

_{Sent with GitHawk}

[bcomnes]

Other things in the choo ecosystem have similar issues (e.g. nanocomponent), I don't have a clear idea idea what the best solution is at this moment. Perhaps a document binding option.

[lukeburns]

Yeah, that probably makes the most sense. Shadow dom / iframes are meant to encapsulate, so it may be tricky business trying to work around that globally.

Is MutationObserver even able to see mutations in shadow nodes / iframes? I could imagine something like onload.addDocument(shadowRoot) that just adds a new mutation observer / handler bound to that shadow root, or iframe.

[bcomnes]

Long term, hooking into web-component native mount hooks seems to be the most straightforward way for components that need an on-load hook. As I'm not actively pursuing a solution at the moment, I'll delegate to someone else (perhaps myself in the future) in the meantime.