effects stacking

[titoBouzout]

There seems to be some sort of bug with clearing effects, stack keeps growing (duplicate console logs).

This behaviour was introduced in https://github.com/solidjs/signals/commit/3b4cf0e44625685d2c24b181c8ffbc2cb6470f5a

The commit before that doesn't stack effects.

Reproduction running npm run build && node tests/createEffect.test.stackbug.js with the following file:

// file: test/createEffect.test.stackbug.js

import { createEffect, onCleanup, createRoot, createSignal } from "../dist/prod.js";

createRoot(() => {
  const [a, setA] = createSignal(1);
  const [b, setB] = createSignal(2);

  setInterval(() => {
    setA(a => a + 1);
    setTimeout(() => {
      setB(b => b + 1);
    }, 1000);
  }, 2000);

  console.log("createRoot");

  const displayEffects = false;

  createEffect(
    () => {
      displayEffects && console.log("createEffect a computed");
      onCleanup(() => {
        console.log("clean up a");
      });
      return a();
    },
    a => {
      displayEffects && console.log("createEffect a effect");

      console.log({ a });

      const [c] = createSignal(3);

      createEffect(
        () => {
          displayEffects && console.log("createEffect b computed");

          onCleanup(() => {
            console.log("clean up b");
          });
          return b();
        },
        b => {
          displayEffects && console.log("createEffect b effect");

          console.log({ b });

          createEffect(
            () => {
              displayEffects && console.log("createEffect c computed");

              onCleanup(() => {
                console.log("clean up c");
              });
              return c();
            },
            c => {
              displayEffects && console.log("createEffect c effect");

              console.log({ c });
            }
          );
        }
      );
    }
  );
});

Reported in https://discord.com/channels/722131463138705510/751355413701591120/1306256252262940743

[ryansolid]

I should write up on this but nested reactivity shouldn't be created in the right hand side. All create and onCleanup calls should be on the left. This implementation lacks guards currently but something to keep in mind. I also haven't implemented cleanup for the right side yet.

It will be possible with Transactions for the left side to run multiple times before the right does, so cleanup needs to be separated. My current thinking is have the right side return it's cleanup function like React.

[titoBouzout]

Thanks, I suspected we weren't using this correctly.

I'm more on the side of using onCleanup instead of returning a function. It will make it consistent with how the rest of things are used. Consistency is welcomed because removes cognitive load. You seem to agree given this comment https://github.com/solidjs/solid/pull/2323#issuecomment-2397533165 but on the other hand, if other apis adopts this approach then it may be fine... something to consider

[titoBouzout]

This has been resolved long ago