Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
TPReal opened this issue Jan 19, 2025 · 7 comments · May be fixed by #2411
Open

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

TPReal opened this issue Jan 19, 2025 · 7 comments · May be fixed by #2411

Comments

@TPReal
Copy link

TPReal commented Jan 19, 2025

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.

TPReal added a commit to mblajek/Memo that referenced this issue Jan 20, 2025
@ryansolid
Copy link
Member

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
Copy link
Author

TPReal commented Jan 21, 2025

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
Copy link
Member

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
Copy link
Author

TPReal commented Jan 22, 2025

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
Copy link
Author

TPReal commented Jan 22, 2025

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
Copy link
Member

@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 added a commit to TPReal/solid that referenced this issue Jan 24, 2025
The `<Show>` component evaluates its `when` condition more often than necessary, in particular it is immediately evaluated twice if the condition is true, children is specified as a function, and keyed is not specified.

solidjs#2406
TPReal added a commit to TPReal/solid that referenced this issue Jan 24, 2025
Adds another memo directly on `when` in `<Show>`.

solidjs#2406
TPReal added a commit to TPReal/solid that referenced this issue Jan 24, 2025
This removes a bug where the `when` condition of a `<Match>` was evaluated twice immediately after creation, when the condition was true, children was a function and keyed was not specified. It also removes any unnecessary conditions evaluations by creating a memo on every `when` in a `Switch`.

For example, if a `<Switch>` has two `<Match>`es with `when={a()}` and `when={b()}` respectively, then:
- `b()` is never called if `a()` is truthy (which was true also before this change),
- `a()` is never called when `b()` changes (which is new).

solidjs#2406
@TPReal
Copy link
Author

TPReal commented Jan 24, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants