Add a definition for a generic signal to web core

[ava-cassiopeia]

This can wrap any framework's implementation of a signal, so that we can let downstream implementations use their native signals instead of always relying on (or wrapping) Preact signals.

[gemini-code-assist]

Code Review

This pull request introduces a FrameworkSignal interface and its test implementations for Angular and Preact signals, aiming to provide a generic abstraction for framework-specific signal handling. It also updates project dependencies to include @angular/core and upgrade rxjs and tslib, alongside a tsconfig.json modification for rxjs/operators path mapping. Feedback includes a high-severity suggestion to enhance the FrameworkSignal interface with a WritableSignalType parameter for improved type safety, which would necessitate an update to the Angular test implementation. Additionally, medium-severity concerns were raised regarding nested rxjs and tslib dependencies in package-lock.json that could increase bundle size, and the explicit rxjs/operators path mapping in tsconfig.json which might indicate a module resolution issue.

[gemini-code-assist]

The AngularSignal implementation for set uses AWritableSignal<T> as the type for the signal parameter, which is appropriate for a writable signal. However, the FrameworkSignal interface (as currently defined) expects SignalType. With the proposed change to FrameworkSignal to include WritableSignalType, this line would need to be updated to use AWritableSignal<any> as the second generic parameter for FrameworkSignal.

This adjustment ensures type consistency with the refined FrameworkSignal interface, making the Angular implementation correctly typed.

    const AngularSignal: FrameworkSignal<ASignal<any>, AWritableSignal<any>> = {

[gemini-code-assist]

The package-lock.json shows nested installations of rxjs (version 6.6.7) and tslib (version 1.14.1) under node_modules/inquirer/node_modules/. This occurs despite newer versions of rxjs (7.8.2) and tslib (2.8.1) being installed at the root level. While npm's dependency resolution handles this, it can lead to increased bundle size and potential for subtle bugs if different parts of the application inadvertently use different versions of the same library.

[gspencergoog]

So, is the idea that there is no default signal type (e.g. preact), and someone implementing a new renderer would be required to implement one of these, so that all signals are "native" to the framework?

[ava-cassiopeia]

So, is the idea that there is no default signal type (e.g. preact), and someone implementing a new renderer would be required to implement one of these, so that all signals are "native" to the framework?

Exactly. In a followup PR, I would set it up such that all of our reactive stuff that currently transacts in Preact signals would instead use a FrameworkSignal implementation to interact with the actual underlying framework signals.

[ava-cassiopeia]

I could also imagine providing a "default" impl of FrameworkSignal that uses preact signals (or whatever we decide is 'generic' or 'default'), but I think in practice any downstream framework probably wants to use their own thing.

[jacobsimionato]

So, is the idea that there is no default signal type (e.g. preact), and someone implementing a new renderer would be required to implement one of these, so that all signals are "native" to the framework?

Exactly. In a followup PR, I would set it up such that all of our reactive stuff that currently transacts in Preact signals would instead use a FrameworkSignal implementation to interact with the actual underlying framework signals.

Cool! I guess the open question then is how to inject the "SignalFactory" etc into all the classes which need to create signals in web_core.

I'd favor putting in the constructor of each object (e.g. MessageProcessor, SurfacesModel, SurfaceModel, DataModel, DataContext etc) and passing it through, seeing as every framework will have a different approach to DI.

One prominent place where Signals are exposed on the API for web_core is FunctionImplementation - see

A2UI/renderers/web_core/src/v0_9/catalog/types.ts

Line 69 in 0273076

): unknown | Signal<unknown>;

. This is a bit unique, because here we let application developers supply us a signal representing the changing output of a function, rather than get a signal that we create. I think we should perhaps add SignalFactory or similar as a parameter of the execute method.

[jacobsimionato]

Hitting approve because my comments are minor - I mostly think it's worth putting this in a more generic location like /signals or /types or /primitives or something.

[ava-cassiopeia]

I'd favor putting in the constructor of each object (e.g. MessageProcessor, SurfacesModel, SurfaceModel, DataModel, DataContext etc) and passing it through, seeing as every framework will have a different approach to DI.

Yeah, the draft I'm working on takes this approach. It also means it forces upstream libraries to explicitly implement this interface, which is nice for consistency.