Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unload only if not mounted #35

Merged
merged 2 commits into from
Aug 10, 2018
Merged

unload only if not mounted #35

merged 2 commits into from
Aug 10, 2018

Conversation

lukeburns
Copy link
Contributor

@lukeburns lukeburns commented Mar 2, 2018

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.

#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
Copy link
Collaborator

bcomnes commented Mar 2, 2018

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

@lukeburns
Copy link
Contributor Author

lukeburns commented Mar 2, 2018

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

@bcomnes
Copy link
Collaborator

bcomnes commented Mar 2, 2018

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
Copy link
Contributor Author

lukeburns commented Mar 2, 2018

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
Copy link
Contributor Author

@shama, do you anticipate this causing complications?

@bcomnes
Copy link
Collaborator

bcomnes commented Mar 6, 2018

@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
Copy link
Contributor Author

all good -- that sounds good to me!

@lukeburns
Copy link
Contributor Author

lukeburns commented Mar 13, 2018

@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
Copy link
Collaborator

bcomnes commented Mar 13, 2018

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
Copy link
Contributor Author

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

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 4, 2018

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

@lukeburns
Copy link
Contributor Author

👍

@lukeburns
Copy link
Contributor Author

Any steps to take before that?

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 10, 2018

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

@bcomnes bcomnes merged commit cec46f1 into shama:master Aug 10, 2018
@lukeburns
Copy link
Contributor Author

all good. thanks!

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 10, 2018

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

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 10, 2018

🎉on-load@4.0.0🎉

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 10, 2018

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

@bcomnes thoughts on shadow and iframes nodes?

Sent with GitHawk

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 11, 2018

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
Copy link
Contributor Author

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
Copy link
Collaborator

bcomnes commented Aug 12, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants