The condition of <Show> runs another time unnecessarily when using the function form

[TPReal]

Describe the bug

It looks like the when condition of the <Show> component is run twice unnecessarily at initialisation when a function as passed as children. It is first run when creating the condition memo, but then it is run again to be passed as the parameter to the children func, instead of passing the condition directly to the children func.

Your Example Website or App

https://stackblitz.com/edit/solidjs-templates-blqgxpec?file=src%2FApp.tsx

Steps to Reproduce the Bug or Issue

1. Use a <Show> component (not keyed), with function passed as children.

The when condition is immediately evaluated twice.

Expected behavior

The when expression is evaluated once, and then reevaluated whenever there's an update notifying the value might have changed.

Screenshots or Videos

No response

Platform

• OS: Windows
• Browser: Chrome
• Version: 1.7.6

Additional context

The problem is especially big if the condition creates some JSX, e.g. when JSX is passed through props. Normally this JSX is not considered to change, even if some signals are updated, but rather it updates its contents reactively, but here two instances of the JSX are created, and the first one is not cleaned up when the second one is created, but instead they coexist (although only one of them is mounted to the document). See the provided stackblitz for an example.

The Show component has this code:

(child as any)(
  keyed ? (c as T) : () => {
    if (!untrack(condition)) throw narrowedError("Show");
    return props.when;
  }
)

It is surprising to me that props.when is passed to child, instead of passing there the already memoised condition created before. This would fix the problem described here, but I'm not able to guess if it creates any other problems.

[ryansolid]

Yeah the reason for this is because condition only exists to trigger the switch to happen. Ie to determine when to re-run the function. The problem is it doesn't change while truthy when non-keyed. So in this scenario where you want to get the latest value at the time you call it we can't read from that memo.

I did debate at one time equality only impacting notification but that is weird because you have different versions of the value around then. So we guard write.

The only way to solve this would be to always memoize the when clause before reading it in the condition or outputting. Obviously that can be done on the outside like you are doing, but there is an argument for bringing it inside in this case (ie.. non-keyed, fn as children)

[TPReal]

Ah, I didn't realise the condition would never change (between truthy values) in this code.

So if I understand correctly, one solution would be two layers of memos inside Show, one with regular equals, just used to access the when value without reevaluating it, and the other as it is now, for non-keyed variant.

Another idea that comes to my mind would be to use the parameter of the function passed to createMemo. Something like this:

createMemo((prev) => {
  const c=condition(); // where condition has the default equals
  if(c)
    return prev||renderTheNewContent();
  else
    return props.fallback;
});

I mean, this is not more than pseudocode, e.g. this wouldn't react to the change of props.children, but looks to me like this approach could be used instead of specifying the equals in the condition, right?

[ryansolid]

It isn't quite that simple because nested things get released on new execution unless they create isolated disposed roots or hoist it to a parent context with runWithOwner. The double memo is probably the best. I didn't implement it originally thinking that most of things of consequence would already be wrapped, but you are right it is a bit unexpected.

[TPReal]

What's really weird is we had the code that created JSX twice for like a year in our project, and never realised that, until recently I added there a component that does some backend requests, and I noticed they get sent twice. So whole this time we had two copies, with all the effects etc running unnecessarily twice, and it's really easy to miss this. Even things like onMount get called, even though the element in condition is never mounted.

There seems to be exactly the same problem with <Match> also.

[TPReal]

I made a draft of a fix for Show, could you please take a look if it's a good solution? If it is, I'll implement the same for Match and prepare a proper PR. If not, sorry :) I accept that my magic level might not be high enough.

#2408

[ryansolid]

@TPReal yeah that looks pretty good. I'm working in future future branch right now but when I get a chance I will take a closer look and merge.

Switch/Match might be harder to do optimally because the keyedness can be per condition. That was probably overkill but now we are here.

[TPReal]

My fix for Show should be good enough I think (unless there's a performance problem with creating that extra memo), I also made a fix for Switch, it complicates things a bit and I would not bet my life on it being correct, so please take a closer look, but the tests pass. It also removes other unnecessary condition evaluations (explained in the commit message), although it's not a significant gain, more like just a fortunate side effect.