Make sure fetches are aborted if ancestors are removed from dom

[gazpachoking]

Before contributing, please read the contribution guidelines. Any change in behavior must be accompanied by appropriate justification, clearly describing the problem, solution, and alternatives considered.

Problem Statement

• @fetches are not aborted if one of their ancestors are removed from the dom rather than the element directly
• @fetches are aborted if their element is removed from the dom, even if they have requestCancellation set to 'disabled' or an explicit abort controller

Proposed Solution

• A global mutation observer checks whether any of the 'auto' fetches are no longer connected to the document and aborts them
• 'disabled' and explicit abort controllers are not tracked at all and never automatically abort

[goertzenator]

I did some testing and found that removing the parent element still resulted in an abort, but removing the grandparent did not cause an abort.

[gazpachoking]

Not sure about the implementation in this PR. Experimented with another way here: develop...gazpachoking:datastar:action-cleanups

This one expands the cleanup mechanism for attributes to store a map of cleanup functions rather than a single one, and that map is passed all the way down into actions where they are free to add and remove their own cleanups. I'm not sure about either of my methods, but there are some plusses with this other one:

• No need for a separate mutation observer, shares the same one that watches for attribute cleanups.
• Abort happens even if the attribute that spawned it is removed, not just when the whole element goes away.
• Abort on new request and abort on attribute removed can share the same cleanup function.

[wmTJc9IK0Q]

@gazpachoking Yes in my original implementation of this in #1045 that was ignored/closed without discussion, I shared the global mutationobserver for the case where an ancestor was removed. The version that got merged doesn't handle this case at all and I would consider broken.

You can handle just the ancestor scenario without the attr removed scenario without so many changes. actually I think my version does handle this too, see next comment

[wmTJc9IK0Q]

Here is a simpler version using the global mutation observer: develop...wmTJc9IK0Q:datastar:auto-cancel-remove

You can use it from a CDN like this:

<script type="module" src="https://esm.sh/gh/wmTJc9IK0Q/datastar@8e12c7ff/library/src/bundles/datastar.ts"></script>

I haven't tested it, but I think this actually will get called when an attribute is removed here: https://github.com/wmTJc9IK0Q/datastar/blob/8e12c7ffac3ec103dd78f4bf6f4adc626f03d6d3/library/src/engine/engine.ts#L171

[bencroker]

Yes in my original implementation of this in #1045 that was ignored/closed without discussion, I shared the global mutationobserver for the case where an ancestor was removed. The version that got merged doesn't handle this case at all and I would consider broken.

@wmTJc9IK0Q your PR was not ignored without discussion. Other people made their cases for this change in behaviour, and I asked you to justify yours. You did so, graciously, we discussed internally, and the team ended up choosing a different implementation.

While I agree that the implementation we chose needs work, just because your version works for you, it doesn’t automatically mean we want to support it going forward. We are listening and paying attention, and yet need to be cautious about what features and changes in behaviour we introduce. Please understand this, and that nobody is “ignoring” you.

[goertzenator]

HTMX has a custom event 'htmx:beforeCleanupElement' that is sent to each element as it is disabled or removed. Would something like that make sense for Datastar?

Such an event would be useful for the fetch plugin, but also any other present and future plugin (internal and external) that needs cleanup. Without such a mechanism each plugin would be forced to write their own MutationObserver machinery which is tricky and duplicates code.

[gazpachoking]

I haven't tested it, but I think this actually will get called when an attribute is removed here:

Ahh, I see. Your version is one level shallower than mine. You create a cleanup for the 'fetch' attribute which only gets called when the whole element is removed (or if an attribute called 'fetch' ended up getting removed from the element.) I went one level deeper and instead of creating a fake attribute name I let there be multiple cleanups per attribute name. More technically correct, but whether it's worth it and if there is a better way is still a question.

[goertzenator]

My understanding of this code is pretty new and could be way off, so take this with a grain of salt:

One possible problem I see that is that the cleanups for multiple @fetches in a single element would stomp on each other (I'm not even sure if multiple concurrent fetches are possible, but I do see how that might be useful.) I think this is also a problem for the existing attribute cleanup mechanism (duplicate attributes with cleanups will stomp on each other).

I have to wonder if a cleanup event might be simpler. engine could unconditionally send a datastar-cleanup event to all elements that are being removed, and any plugins (attribute or action) can listen for it if they need cleanup. Basically all cleanup tracking is externalized to DOM listener machinery. edit: Hmm, I see that attributes can come and go without an attribute being removed, so maybe this is not the way.

[wmTJc9IK0Q]

@goertzenator I wouldn't waste your time here - the maintainers don't engage in discussion on implementation or tradeoffs. They prefer to discuss amongst themselves only.

[gazpachoking]

I'm not even sure if multiple concurrent fetches are possible, but I do see how that might be useful.

Not with the abort set to auto, which is the case here.