lit / lit

Lit is a simple library for building fast, lightweight web components.
https://lit.dev
BSD 3-Clause "New" or "Revised" License
18.52k stars 914 forks source link

[@lit-labs/preact-signals] Memory leak if element updates while disconnected #4735

Open sorvell opened 3 weeks ago

sorvell commented 3 weeks ago

Which package(s) are affected?

Other/unknown (please mention in description)

Description

In uncommon scenarios, it's possible to cause an unexpected and undesirable memory retainer if a disconnected element updates and accesses signals.

In this case, any subsequent sets to these signals will provoke the element to update and/or set the DOM bound to the signals. This will prevent the element from being garage collected as long as the provoking signals are alive, even when no other reference to the element exists.

Consider the following:

const s = signal(0);
let count = 0;
@customElement('my-element')
class MyElement extends SignalWatcher(LitElement) {
  render() {
   console.log('rendered', ++count);
    return html`${s}`
  }
}

//...
let el = document.querySelector('my-element');
s.value++;
await new Promise(requestAnimationFrame); // console: rendered 1 (expected)
el.remove();
s.value++;
await new Promise(requestAnimationFrame); // console: empty (expected)
el.requestUpdate();
await new Promise(requestAnimationFrame); // console: rendered 2 (expected)
s.value++;
await new Promise(requestAnimationFrame); // console: rendered 3 (expected)
el = undefined;
// provoke garbage collection
s.value++;
await new Promise(requestAnimationFrame); // console: rendered 4 (not expected)

Ideally, in this case when either SignalWatcher or watch is used, since the element is otherwise not reachable, it would be garbage collected and the setting the signal would no longer provoke it to update.

This behavior should be possible by using a WeakRef to access the element/directive in the effect that reacts to the signal change.

Reproduction

reproduction

Workaround

N/A

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

3.x

Browser/OS/Node environment

All

sorvell commented 3 weeks ago

Prototype of potential fix.