Please avoid using WeakRef in your code

[wolfbeast]

After running into an issue with Gitea and their devs tracing it back to github text-expander and then to here as a dependency, I'd like to point out the following:

dom-input-range/src/input-style-clone-element.ts

Line 72 in 5e51f15

this.#inputRef = new WeakRef(input);

this.#inputRef = new WeakRef(input);

Using WeakRef in any capacity outside of extremely specific situations (and tailor-made code) is strongly discouraged. e.g. the W3C TAG Design Principles group has a number of cautionary notes about it, and in many cases calling the WeakRef API and creating the associated finalization objects on resources is not helping with GC but rather a hindrance. It should never be incorporated in generic libraries or broadly-used dependencies.

It's also entirely unclear why you chose to use WeakRef in this situation to begin with; I think it doesn't do what you suppose it does. Calling .deref() does not immediately GC it nor does it guarantee anything about how safe the object is to garbage collect. Your assumption that the input gets garbage collected on unmount immediately is incorrect. .deref() is also not a magic wand to avoid having to properly release references to objects, as finalizers may or may not be called after .deref()-ing an object. At most they are hints to the garbage collector but any existing (stronger) ref won't be blown away by this.

Calls to WeakRef's CTOR are also not supported by all browser engines (which is what started the Gitea issue to begin with), and where they are supported, they are not at all reliable.

You may want to revert (part of) commit b220c7e and avoid calling the WeakRef API in the future.

[iansan5653]

Hello, sorry for missing this issue earlier. The usage of WeakRef here is very intentional because of how this class attaches to and observes a DOM element. This DOM element may then be removed from the page, at which point there's no reason to keep the element in memory. Except that the observer class InputRange will continue to refer to the element, preventing it from being garbage collected. WeakRef is the easiest way to indicate to the GC that this class doesn't need to keep the target element in memory.

Calling .deref() does not immediately GC it nor does it guarantee anything about how safe the object is to garbage collect.

I understand this. deref is the method used to access the object stored by the WeakRef. It has nothing to do with indicating whether garbage collection may occur. The use of WeakRef in the first place is what indicates that the object is safe to collect (assuming all other callsites also have released the object).

At most they are hints to the garbage collector but any existing (stronger) ref won't be blown away by this.

I understand this as well. The class is not trying to imperatively garbage-collect the object, it's simply trying to avoid preventing the object from being held longer than necessary.

Based on this, I don't feel the need to prioritize removing WeakRef from my code. It's a standard API that is widely supported, and it's the simplest way to accomplish this task.

That being said, I can see that there are other ways to accomplish this task. We could instead check to see if the element is still present in the DOM, and if not, clear the reference to release it. While I don't have the time to work on this myself right now, I'd be happy to review a PR.

[wolfbeast]

Thanks for looking into this.

The usage of WeakRef here is very intentional because of how this class attaches to and observes a DOM element. This DOM element may then be removed from the page, at which point there's no reason to keep the element in memory. Except that the observer class InputRange will continue to refer to the element, preventing it from being garbage collected.

I'm afraid you are mistaken. Observers do not keep objects they observe "alive" like a strong ref would; they will simply stop observing the element and never trigger again if the observed element is removed. You don't have to do anything else for the GC to clean up. You don't need to perform manual clean up here and there's no need to use the API which isn't as widely supported as you assumed. Conversely, if your assumption would be correct then using the API would not help your cause, because the GC in that case would not be able to dereference the element if there would still be an observer as it would break a reference to a "live" element which it should not do. i.e.: the API does not, in fact, change the behaviour of the GC; it merely hints to it that if it is not sure (e.g. nearing its trace limit or dealing with complex "islands") that it could collect the object, instead taking the cautious approach.

e.g.: from MDN talking about using .disconnect():

If the element being observed is removed from the DOM, and then subsequently released by the browser's garbage collection mechanism, the MutationObserver will stop observing the removed element.

So I'm afraid your use of WeakRef() is just ill-advised and unnecessary; even more so when dealing with what are really small and simple allocations to begin with, and in typical use on short-lived containers. You could simply remove the usage of the API and it will not have negative consequences.

[iansan5653]

Observers don't keep objects alive, but a strong reference to the element in a property of the InputRange class absolutely would. Which is exactly why InputRange only carries a weak reference to the element.

[iansan5653]

If you feel that strongly about not supporting it, have you considered just shimming the API in your browser project so that you could safely render sites that use it? Garbage collection is opaque enough that no site is ever going to break if the WeakRef they are using doesn't actually store a weak reference.