0.33.7 causes infinite reactivity loops with any custom elements that happen to read a reactive variable in their constructor

[trusktr]

Before 0.33.7, things worked fine in my case because reading a reactive variable in a constructor wasn't triggering reactivity due to the use of cloneNode inside of dom-expression's get children getter.

Code is similar to this:

@element('my-el')
class MyEl extends Element {
  @attribute foo = "bar" // @attribute creates a signal-backed property

  constructor() {
    this.somethingElseWithInitialValue = this.foo + "baz" // infinite loop, triggers `get children` again
  }
}

A workaround for me for now was to downgrade to 0.33.0. I'll need to update elements to ensure they don't read reactive properties in their constructor (easy thing to do, especially in cases where some initial logic depends on them).

Would it make sense to change the compiler output to the following with untrack (or some other way to make the importNode call untracked)?

const _el$123 = untrack(document.importNode(_tmpl$123))

The infinite loop re-runs the children getter repeatedly.

[trusktr]

I am planning to release more decorators in classy-solid, and some of them will depend on constructor initialization. For example:

class Foo {
  @signal count = 123
  @memo double = this.count * 2 // this initially runs in the `constructor` as per ES spec.
}

I can use an untrack to wrap the decorator's initializer. Would it make more sense to just untrack the template instantiation so lib authors don't have to use untrack? It wouldn't make sense for element instantiations to be reactive anyway.

[trusktr]

I may not have analyzed my case deeply enough: maybe the constructor call is both reading and writing to the same signal property actually.

In concept, maybe something like this:

class Foo {
  @signal count = 123
  @memo double = this.count * 2 // this initially runs in the `constructor` as per ES spec.

  constructor() {
    // ...initializers run here...
    if (foo) this.count = 456 //  in some case, start with different value.
  }
}

which is totally plausible. This also leads me to believe that untracking the importNode might be the simplest best solution.

---

Another question is, why is get children reactive? Maybe there's more to it than I've analyzed so far...

---

Another consideration is that I've been thinking about creating effects in constructors, so that things are initially reactive even when not connected to DOM, to avoid inconsistent behavior when someone is using an element as a data container prior to appending it.

I.e. the existence of reactivity shouldn't depend on an element's connectivity into the DOM, I think (needs experimentation at least).

The cloneNode approach would have been fine with that too, but wth importNode there will be effects in effects.

[ryansolid]

This was to fix getter/setters etc in custom elements. I couldn't figure out why it was always working properly in Lit and not Solid. This effectively lets us upgrade before connectivity that fixes all the weird behaviors I was seeing. I think you may have reported some of them in the past. Like people would define getter/setters and then try to assign to them and Solid would write before update, basically blocking the prototype on future updates.

I'm interested a bit more on an example of how this happens because usually we aren't cloning(importing) nodes in a reactive context. Also any memo/effect creation will isolate from the parent scope and should not cause infinite loops. I don't like using importNode because its slower, but it seemed the lesser evil since I think we do want to upgrade before connect to make things work consistently.

[trusktr]

Solid would write before update, basically blocking the prototype on future updates.

This is exactly the problem Custom Element authors need to write robust code against. Custom Element authors need to fix this issue in their custom elements (making them robust against upgrade timing) regardless if Solid side steps the issue in this particular case or not.

The thing about this fix, is that it "fixes" this issue only for people who are writing custom elements and testing them only inside of Solid components.

The moment any other non-Solid app writes values to a pre-upgraded custom element for any reason (it will happen!) Solid's change to importNode doesn't come into play at all.

Whoever this fix is for, they need to be aware that they haven't fixed the problem, and that this issue is only side stepped while they're testing in Solid.

@element('my-el')
class MyEl extends Element {
  @attribute foo = "bar" // @attribute creates a signal-backed property

  constructor() {
    this.somethingElseWithInitialValue = this.foo + "baz" // infinite loop, triggers `get children` again
  }
}

---

I'm interested a bit more on an example of how this happens because usually we aren't cloning(importing) nodes in a reactive context.

I performed the untrack experiment we chatted about in the Discord chat.

First I'll describe the problem, then show the workaround. This is the problematic code:

@element('my-el')
class MyEl extends Element {
  @attribute foo = {bar: 'baz'} // @attribute creates a signal-backed property, with `{equals: false}`

  constructor() {
    super()

    // Some reactive code that reads and writes to the same signal:
    const foo = this.foo // read .foo
    foo.bar = "someValue"
    this.foo = foo // write .foo
  }
}

This triggers this effect in Solid's Show component in my case:

function Show(props) {
  let strictEqual = false;
  const condition = createMemo(() => props.when, undefined, {
    equals: (a, b) => strictEqual ? a === b : !a === !b
  });
  return createMemo(() => {
    const c = condition();
    if (c) {
      const child = props.children; // <----------------------------- HERE, calls `importNode`
      return (strictEqual = typeof child === "function" && child.length > 0) ? untrack(() => child(c)) : child;
    }
    return props.fallback;
  });
}

The call to props.children calls importNode which creates a new element, and that element subsequently reads and writes foo within the same memo effect of the Show component.

Each time an element is created, it is a new .foo property with a new signal behind it. So it loops infinitely, although the signal is different on each effect run.

To work around the problem on my end, I can do this:

@element('my-el')
class MyEl extends Element {
  @attribute foo = {bar: 'baz'} // @attribute creates a signal-backed property, with `{equals: false}`

  constructor() {
    super()

    untrack(() => {
      // Some reactive code that reads and writes to the same signal:
      const foo = this.foo // read .foo
      foo.bar = "someValue"
      this.foo = foo // write .foo
    })
  }
}

Now it no longer loops.

I could abstract that workaround into the @reactive decorator like so:

function reactive() {
  return class {
    constructor() {
      return untrack(() => new Class())
    }
  }
}

but with the @element decorator it is much more difficult because we need to avoid the Illegal constructor error from the DOM engine.

best solution:

I think we should wrap the importNode call in untrack. Here's why:

With DOM APIs custom elements are always constructed in a non-reactive context and do not track any dependencies (because the DOM engine doesn't use Solid). Therefore I think that Solid should follow the pattern of custom element construction not being reactive with untrack around importNode.

[edemaine]

I think we should wrap the importNode call in untrack.

This sounds analogous to createComponent wrapping component functions in untrack. I don't know WC well enough to say for sure, but it sounds like a good idea.