ReviewOS

stacks/stx

buildComponentLibrary: a compiled element has no way to clean up, so anything with an observer or a document listener cannot be written

#2010
Closed glennmichael123 opened this 9 days ago ·
9 days ago

buildComponentLibrary gives a component props and a render. There is no hook that runs when the element leaves the DOM, and the name is reserved, so a component cannot release anything it acquired:

resources/elements/weight-chart.stx: method "disconnectedCallback" is reserved

connectedCallback is reserved the same way. An instance property is not an alternative — the browser reads custom-element reactions off the prototype at define() time, so this.disconnectedCallback = ... assigned during a render is never called.

What that rules out

Work that starts from a render can be made to work: the fetch, caching on this, requestUpdate() when it lands, and a queueMicrotask for the few things that need the written markup. I converted one element that way and it matches the hand-written original exactly.

The next one cannot be written at all. It holds:

disconnectedCallback() {
  if (this._observer) this._observer.disconnect()        // content changes
  if (this._resize) this._resize.disconnect()            // redraw at a new width
  if (this._themeObserver) this._themeObserver.disconnect()  // follows the page's dark class
  this._release()                                        // document + window listeners
}

and, inside the chart it draws:

document.addEventListener('pointerdown', outside, { passive: true })
window.addEventListener('scroll', hide, { passive: true })

Installed from a render, every one of those outlives the element. The document-level listener is the sharp end: it holds the element and its whole shadow tree alive after removal, so an element created per page view in a SPA leaks without bound. There is no way to write it correctly, and a compiled element that leaks is worse than the hand-written one it would replace — so that conversion is where I stopped.

Suggestion

Unreserve the two lifecycle names and call them around the runtime's own, which is what a subclass would do:

connectedCallback() { super.connectedCallback(); /* component body */ }
disconnectedCallback() { /* component body */; super.disconnectedCallback() }

Or, if the point of reserving them is that the runtime owns the sequence, an explicit pair the runtime invokes — onConnected / onDisconnected — would do the same job without letting a component break the base class.

A @connected / @disconnected block in the .stx file would be nicer still, since it keeps the component's lifecycle in the component rather than in a build config.

Related: stacksjs/stx#2009, which is about the same build's defineProps handling.

9 days ago

Measured this properly rather than reasoning about it, on stx 0.2.343. Three things came out of it: the leak is per render rather than per element, one claim in the issue above is overstated, and the component half of the fix already compiles today — so this is a two-line runtime change, which I've now measured end to end.

The component body runs on every update

This is the part I had wrong in my head. componentModule inlines the component's <script> body inside the generated render():

const inner = [
  destructured ? `const { ${destructured} } = this._props();` : '',
  scope.body,          // <- the component's own script
  "let out = '';",
  component.render,
  'return out;',
]

and _render() calls this.render(RENDER_HELPERS) on every update. So anything the body installs is installed again on each one. A minimal element:

<script>
  globalThis.__runs = (globalThis.__runs || 0) + 1
  const handler = () => { globalThis.__fired = (globalThis.__fired || 0) + 1 }
  document.addEventListener('pointerdown', handler)
  const label = `body runs: ${globalThis.__runs}, nudge=${nudge}`
</script>

<div class="probe">{{ label }}</div>

compiled with buildComponentLibrary, then three attribute changes, one document.dispatchEvent(new Event('pointerdown')), remove(), and one more dispatch:

body runs after first render1
body runs after 3 attribute changes4
document listeners responding while connected4
document listeners still responding after remove()4
the runtime's own delegated listeners after remove()0

So it is not one stranded listener per element, it is one per render. An element that redraws on resize — which is why mine observes its own width — accumulates them for as long as the page lives.

That last row is the part worth drawing out: the runtime already solves this problem for itself. _bindEvents records what it attaches in this._listeners, and disconnectedCallback removes them. The machinery and the intent are both there; a component just has no way to join in.

Correcting the issue above

Installed from a render, every one of those outlives the element.

That overstates it, and the distinction matters for anyone triaging this. A MutationObserver or ResizeObserver that observes only the element itself forms a cycle with it and is collectable once the element is unreachable — those are untidy, not leaks. What actually strands is anything registered on a root: document, window, or document.documentElement (my theme="class" observer watches <html>, so it is in this group). The document listener above is measured; the collectability of the self-scoped pair is reasoning, and the measured half is already enough to block the conversion.

The instance-property workaround is provably dead

Asserted above from the spec; here it is measured, so nobody has to take it on faith:

const el = document.createElement('probe-leak')
el.disconnectedCallback = () => { calledOwn++ }
document.body.appendChild(el); await el.updateComplete; el.remove()
// → { ownIsOwnProperty: true, instanceHookCalled: 0, prototypeReactionRan: true }

The own property is there, the reaction fires, and the own property is never consulted — reactions are looked up on the prototype when define() runs.

The fix is two lines, and the component half already works

RESERVED_METHODS holds the four reactions and constructor. onConnected / onDisconnected are not in it, so a <script client> function by that name already compiles into a prototype method on the generated class today — I checked the output:

/** FixedProbe web component generated from fixed-probe.stx. */
class FixedProbe extends StxElement {
  onDisconnected() { ... }

Nothing calls it. The whole gap is that StxElement invokes this.render and nothing else. Patching my copy of the generated runtime.js with exactly this:

   disconnectedCallback() {
+    this.onDisconnected?.();
     this._connected = false;
     for (const [event, listener] of this._listeners) this.renderRoot.removeEventListener(event, listener);
     this._listeners.clear();
   }
       this.dispatchEvent?.(new CustomEvent('stx:hydrated', { bubbles: true, composed: true }));
     }
+    this.onConnected?.();
   }

and re-running the probe with the install guarded on this and a _release the hook calls:

body runs4
document listeners while connected1
onDisconnected ran on removaltrue
document listeners after remove()0

?.() keeps it free for every component that defines neither, and ordering it before _connected = false lets a hook still read the element's state. If onConnected should fire on re-entry rather than only the first connection, that is the one judgement call in the patch.

What people will have to write meanwhile

Since the body re-runs, the install has to be guarded on this, and the only teardown trigger available is the handler noticing it is orphaned:

if (!this._bound) {
  this._bound = true
  const handler = (event) => {
    if (!this.isConnected) return document.removeEventListener('pointerdown', handler)
    /* ... */
  }
  document.addEventListener('pointerdown', handler)
}

Bounded, and it does eventually unregister — but it holds the element and its shadow tree alive until the next event of that type anywhere on the page, and it cannot work at all for an observer, which has no event to piggyback on. Which is why I'd still rather have the hook.

A @connected / @disconnected block in the .stx file remains the nicer shape, since it keeps the lifecycle in the component rather than in a build config — but the two lines above unblock it without any new syntax.

9 days ago

Fixed in 162a0ce6fb.

Reproduced first

defines disconnectedCallback   THREW: client method "disconnectedCallback" is reserved
defines connectedCallback      THREW: client method "connectedCallback" is reserved
defines onDisconnect           emitted as a class method: true   runtime ever calls it: false
defines onConnect              emitted as a class method: true   runtime ever calls it: false

So both halves were real: the reactions are refused, and a differently-named hook was compiled into the class and then never invoked. There was genuinely nowhere to put the work.

What changed, and what deliberately did not

The two reactions stay reserved. Allowing them was the obvious fix and it is the wrong one: the generated element defines both, so a component that redefined disconnectedCallback would take over the framework's listener cleanup and lose it unless the author remembered super.disconnectedCallback(). That is the kind of discipline that fails silently, which is the same class of problem this issue is about.

Instead the element calls hooks from inside its own reactions:

  • onConnect() after it is connected, rendered and bound, so a component acquiring something sees its own markup and its events already in place
  • onDisconnect() as it leaves the DOM, before the framework releases its own listeners, so the element is still intact on the way out, and inside try/finally so a throwing hook cannot leak them

Both are reported rather than rethrown, so a component that fails to set up does not take the element down with it.

The reserved-name error now names the hook instead of just refusing:

Widget.stx: client method "disconnectedCallback" is reserved; define "onDisconnect"
instead, which the element calls as it leaves the DOM, before it releases its own
listeners

Your example now writes as-is, with disconnectedCallback renamed to onDisconnect:

function onDisconnect() {
  if (this._observer) this._observer.disconnect()
  if (this._resize) this._resize.disconnect()
  if (this._themeObserver) this._themeObserver.disconnect()
}

Verification

Checked in a real DOM rather than by matching the generated source, since "the string is present" is not the thing in doubt. An element is appended, removed and re-appended, and the hooks are counted on each transition, which is what an observer re-established on connect and torn down on disconnect depends on. The reserved-name refusal is asserted on its message.

13 tests added across this and #2009, 10 of which fail without the change. tsc --noEmit clean, lint clean, the 34 web-components tests pass.

Sign in to comment on this issue.