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 3b4cf0e

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 solidjs/solid#2323 (comment) but on the other hand, if other apis adopts this approach then it may be fine... something to consider