[🐞] Qwik doesnt check if-statements before running child useComputed$ or useTask$

[DustinJSilk]

Which component is affected?

Qwik Runtime

Describe the bug

The following code causes Qwik to crash when the button is clicked. Conditionals aren't checked before child component tasks are run when using Signals.

import { component$, useSignal, useTask$ } from "@builder.io/qwik";

// A useTask$ in a child components circumvents the parent components conditional checks
const Child = component$((props: { val: string }) => {
  useTask$(({ track }) => {
    track(() => props.val)
  });
  return <>{props.val}</>;
});

export default component$(() => {
  const sig = useSignal<{ data: string } | undefined>({ data: '' });

  return (
    <>
      <button onClick$={() => (sig.value = sig.value ? undefined : { data: '' })}>Toggle</button>

      {/* ERROR: cannot read data of undefined */}
      {sig.value && <Child val={sig.value.data} />}
    </>
  );
});

Reproduction

https://github.com/DustinJSilk/qwik-issue-rendering/blob/main/src/routes/index.tsx

Steps to reproduce

Run the app
Click the button

System Info

System:
    OS: macOS 12.0.1
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 116.35 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 18.12.1 - ~/.nvm/versions/node/v18.12.1/bin/node
    Yarn: 1.22.18 - /usr/local/bin/yarn
    npm: 7.13.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 113.0.5672.126
    Firefox: 113.0.1
    Safari: 15.1
  npmPackages:
    @builder.io/qwik: ^1.1.4 => 1.1.4 
    @builder.io/qwik-city: ^1.1.4 => 1.1.4 
    undici: 5.22.1 => 5.22.1 
    vite: 4.3.5 => 4.3.5

Additional Information

No response

[DustinJSilk]

I keep running into this issue, am I doing something wrong here?

[cmbartschat]

I wish this bug had more attention!

Here's a playground example just to show how bananas this behavior is 🙈

https://qwik.builder.io/playground/#v=1.2.13&f=7Va7CoNAEOzzFRubUwiBFIEgnqn8iJQaErTR4AMC4r9ndvfUK1LYBqIgKnd7t3uzM%2BOB5nxZ0vFBc1jVV16F17YgSTNX4rRewGTk3hke8ewJpjQULzHXZhHl8N6%2Fl4oR4esokusQKZOOEiuSAdWTNJL%2BJGstmSKvcXdGT5qvvmzB2awRWds2bWhuzUAdBBIbrXFOZQ66fcHVQCjd7L3RFSY8p2jntQK7IhVBcFgwestPgXom9Ma27tKCua2vVQ8NFvcg49CSlKc0Y3mnwG0yoJ7dAewVpwVwnFKeh6GV%2BKECPBNLeDvqDhkfMkK%2Fv5WNriotMSV6lH4AlyjCcGGAq%2BhPBD9KBB8

[wmertens]

@cmbartschat I don't understand, what are you demostrating? You're throwing on purpose there?

The "problem" is that the {sig.value && runs at a different time than the updating of the passed signal prop function sig.value.data.

I don't consider this a bug per se, it's more of an unexpected result of splitting your app in tiny parts that update via signals.

[DustinJSilk]

Why wouldn’t it be a bug? Signals are unavoidable and so is splitting an app into smaller parts. So surely it’s a bug if the result is unexpected? Please let me know if I’m missing something fundamental here :)

[wmertens]

Hmm.

So a solution would be that the optimizer keeps track of statements that cause a signal to be forwarded, converting then to a filter function, right? And it should first call the filter function and only then the signal function?

I wonder how deep this rabbit hole goes, what statements should be converted etc.

[DustinJSilk]

To be honest, I don’t know what a solution would look like, I don’t know the inner workings of qwik or what the various trade offs would be.

Do you know if any other framework has solved or suffers from the same issue?

[wmertens]

I don't know of any other framework that converts values into signals and re-renders asynchronously thanks to that.

The workaround is to add a ? after signal.value, which the linter doesn't complain about because it doesn't know that it will become a separate function called separately.

For me this feels like a P1 issue, maybe P2 because it's unintuitive.

[DustinJSilk]

Yep so the only real work around is to avoid the conditional completely, it kind of is bananas lol

Is there any way we can get this issue escalated?

[cmbartschat]

Sorry for the cryptic example!

What I was trying to show is that based on the contract of the Child component, as long as you don't pass "bananas", everything is fine. And naively, it does seem like we're following that contract in that example.

Here's another example to show the sorts of things a real world app might try to do.

I would describe the problem simply that type narrowing does not work on signals.

No matter what we write in the render logic, at runtime, the signal is not type narrowed, because it gets flushed through before the narrowing logic can run.

This can be a source of bugs that Typescript cannot detect.

[wmertens]

Ok your real-world example is convincing. It's easy to see that this could lead to frustrating debug sessions.

So indeed I think the proper solution is that the optimizer supports filter functions before updating prop signals. But I don't think that's trivial to add.

[DustinJSilk]

@manucorporat @mhevery is there anything we can do about this issue?

[mhevery]

Yeah, this is a hard one. I need to dig deeper into this to see what a fix could be.

The issue is that the order in which we process events is not preserved.

[wmertens]

Does this happen in other situations than when signals are proxied in props by the optimizer? If not, I believe a filter function should work

[mhevery]

Does this happen in other situations than when signals are proxied in props by the optimizer? If not, I believe a filter function should work

I have seen other issues on this topic, but I can't find them now.

The issue is that the effects should run in the correct order and then prune its children. I think the code should work as written, but we don't process the effects in the correct order which results in this issue. Happy to have a chat if you want to dig deeper into this.

[DustinJSilk]

Thanks @mhevery and @wmertens i appreciate you guys taking the time to think about this one!

[brandonpittman]

I'm getting a similar issue where due to conditional rendering I'm getting two of the same component in the DOM—one hidden and one not hidden. Both components' useTask$ calls are being fired when the signal value they're tracking changes. Since I'm updating a store value in that task, a double store value change happens.

[wmertens]

@brandonpittman that's by design, because children and parents render independently.

So if you render items in slots, they will always render and update even when the slot is not visible.

[brandonpittman]

@wmertens

So if you render items in slots, they will always render and update even when the slot is not visible.

That was the conclusion I came to. I created a <Show> component similar to Solid.js' but it's causing more trouble than it's worth. I wanted to embed the show/hide logic inside child components, but it seems like I can't do that without unintended consequences.

---

If the projected content isn't displayed at render, it'll go into a q:template. And if that project content is subject to dynamic updates that change the structure of the slotted HTML, it's possible the initial server q:template can exist in the DOM next to a dynamically created element that gets placed in the slot?

[wmertens]

If the projected content isn't displayed at render, it'll go into a q:template. And if that project content is subject to dynamic updates that change the structure of the slotted HTML, it's possible the initial server q:template can exist in the DOM next to a dynamically created element that gets placed in the slot?

Uhm, yes? Not quite sure what you mean, a playground repro would be nice.

[DustinJSilk]

Looks like this will be fixed in V2 🫨❤️
Is there a roadmap for V2 at all?

[tri2820]

This simple example shows how this bug breaks code that appears to work in Qwik. TypeScript does not show any warnings or errors either.

For useTask$, when I switch to useVisibleTask$ it works (because all input signals are correctly their type at that point).
For useComputed$, could the solution be to allow it to run after rendering, like useVisibleTask$?

const _  = useVisibleComputed$(() => {

})

This way the computation is not async anymore, but at least it's a safe alternative to circumvent situation like this.

[wmertens]

This bug is still present in v2, likely because the qwikloader doesn't add events to the scheduler yet

https://qwikdev-build-v2.qwik-8nx.pages.dev/playground/#f=7VZNa8JAEL37K6Ze1mLIsZSA9tBTL6WU3stqNhIwClEPEve%2FO1%2FuriGIV0EFMZud2Z03M29eUjTvb4NFk8Xpy39DMd%2BsJk7sj2uJdGYccVW3u%2F0%2F0VKhYiCjVdvU6%2BPVMsI8EtTEnsP%2FtG2JjtKqVP2BeoG3FZfjfABZvFTIjHwA2l81Y8wGQF3B5IU95uJOCoM%2BqYwwdAGyhqVFhimZ%2BYnhUlPzqpaa5PHClmNe8vyry6lJHtGBKRj8TnvvI07og7otdiMJMwYpgy%2BwDXQh4hxJ6uC8aDdp0rv6XICTowU1lV8K8Qlw1roKKb%2BcTwSnNL3mr60NZbeXX%2FO9OhzdxgxGIKUp4jJqqi5NEuiVJCy62OUWgq6f%2F7pmi7NStgWpOuLXgz4%2BYr%2F0q032zbp0v8eOgiLa0MVxcOGUxQm22YaDQ0CUcX18UuRjUuQZ

[berli888]

how is this a bug ? according to documentation, props are immutable.
https://qwik.dev/docs/components/overview/#props

Props are shallowly immutable, meaning primitive data types (strings, numbers, booleans) cannot be changed once passed. However, internal elements of reference types (objects, arrays, functions) can be changed despite the reference itself being immutable.

The only way I found to overcome this limitation is to pass the signal directly and run again the condition into the child component.

[DustinJSilk]

Yep, currently you'll need to add the conditions and handle the failures in the child components. Im sure once v2 is out there will be capacity to have this resolved.

[Varixo]

should be fixed by #7088 in v2

[DustinJSilk]

You are a HERO @Varixo. I’ll start testing the alpha versions with my app