Create signal implementations that satisfies FrameworkSignal. Simplify SignalKinds.

[ditman]

Description

Make SignalKinds just a holder for "readonly" and "writable" signal types, and use that in the FrameworkSignal type.

Users need to define their FrameworkSignal type as satisfies FrameworkSignal to prevent TS from erasing the internal types (similar to what we do with the component props!).

Split tests into 3 files: 2 for per-framework initialization, and a common one that runs the same tests across both implementations. The verification code remains the same as in the OG commit.

There's some shenanigans with tsc merging all our test files together and seeing the Preact and Angular types colliding with each other, and tests are not exactly identical to what users would define, but this is the general idea.

(I don't know if this does everything we want though!)

Pre-launch Checklist

• I signed the CLA.
• I read the Contributors Guide.
• I read the Style Guide.
• I have added updates to the CHANGELOG.
• I updated/added relevant documentation.
• My code changes (if any) have tests.

If you need help, consider asking for advice on the discussion board.

[gemini-code-assist]

Code Review

This pull request refactors the FrameworkSignal abstraction and its type-mapping system to use conditional types, simplifying how different signal implementations are integrated. It also introduces a shared test suite and specific implementations for Angular and Preact signals. The review feedback identifies that the effect implementations for both Angular and Preact do not correctly handle the cleanup callback and recommends adding comprehensive test coverage for the effect function within the shared testing utility.

[gemini-code-assist]

The effect implementation doesn't correctly match the FrameworkSignal interface. The cleanupCallback is defined as optional in the interface (cleanupCallback?: () => void), but it's required in this implementation. This will cause a type error and runtime issues if effect is called without a cleanup callback.

Please update the implementation to handle an optional cleanupCallback.

  effect: (fn: () => void, cleanupCallback?: () => void) => {
    const e = effect(cleanupRegisterFn => {
      if (cleanupCallback) {
        cleanupRegisterFn(cleanupCallback);
      }
      fn();
    });
    return () => e.destroy();
  },

[gemini-code-assist]

The current implementation of effect for Preact signals doesn't support the cleanupCallback from the FrameworkSignal interface. It completely ignores the second argument.

The effect function from @preact/signals-core supports a cleanup mechanism by allowing the function passed to it to return a cleanup function. You should adapt the implementation to use this feature.

  effect: (fn: () => void, cleanupCallback?: () => void) => effect(() => {
    fn();
    return cleanupCallback;
  })

[ditman]

@ava-cassiopeia what do you think about this feedback? How picky should we be with these test implementations? (I guess people may copy-paste them and then encounter bugs?)

[gemini-code-assist]

The shared tests for FrameworkSignal implementations are missing coverage for the effect function. According to the repository's style guide, code changes should have tests. Since the effect function is a key part of the FrameworkSignal interface, it should be tested to ensure all implementations behave correctly, especially regarding cleanup logic.

I suggest adding a test case for effect.

    });

    it('handles effects and cleanup', () => {
      const signal = SignalImpl.wrap(0);
      let effectCount = 0;
      let cleanupCount = 0;

      const stopEffect = SignalImpl.effect(
        () => {
          SignalImpl.unwrap(signal); // depend on signal
          effectCount++;
        },
        () => {
          cleanupCount++;
        }
      );

      assert.strictEqual(effectCount, 1, 'effect should run once on creation');
      assert.strictEqual(cleanupCount, 0, 'cleanup should not run on creation');

      SignalImpl.set(signal, 1);
      // Effect should run again, and cleanup should run once before it.
      assert.strictEqual(effectCount, 2, 'effect should re-run on dependency change');
      assert.strictEqual(cleanupCount, 1, 'cleanup should run before effect re-run');

      stopEffect();
      // Cleanup should run on stop.
      assert.strictEqual(cleanupCount, 2, 'cleanup should run on stop');

      SignalImpl.set(signal, 2);
      // Effect and cleanup should not run after stopping.
      assert.strictEqual(effectCount, 2, 'effect should not run after stop');
      assert.strictEqual(cleanupCount, 2, 'cleanup should not run after stop');
    });

[ava-cassiopeia]

Thanks for putting this alternative together - do you think we should present this to the core a2ui team and see what they think?

[ditman]

If you're dialing in today, let's bring it up? I'm not sure who else is going to have opinions about this other than us? Maybe @gspencergoog?

[ava-cassiopeia]

I'm strongly against code reuse in tests like this, as it actively buries the lede in tests, following the philosophy in go/tott/643.

[ditman]

Yeah, I know, I just did this to "prove" (by the test construction) that both implementations behaved the same. Since the specific implementations themselves (preact and angular) are not that important, and the testing logic is not that big, I guess we could just duplicate the test cases on each file; I don't mind!

[ditman]

Unfortunately, there's a usability snag in this solution, so we're going with @ava-cassiopeia's!

[ditman]

This is a usability problem. The way these two types are inferred is by extracting the "readonly" and "writable" properties from the SignalKinds that the user is defining.

HOWEVER, the SignalKinds type can't enforce that users define them with readonly and writable; users can do:

declare module './signals.js' {
  interface SignalKinds<T> {
    foo: PSignal<T>;
    bar: PSignal<T>;
  }
}

The type system won't flag the SignalKinds definition as invalid; instead this will fail silently and upon using the signals all the types will be unknown (this is bad). (Imagine this with a less obvious typo!).