Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the FrameworkSignal interface to support type-safe signal implementations via module augmentation and introduces a new effect method. The review feedback identifies inconsistencies in the test implementations of the effect method for both Angular and Preact, specifically regarding the handling and typing of the optional cleanupCallback parameter.
| // | ||
| // declare module '../reactivity/signals' { | ||
| // interface SignalKinds<T> { | ||
| // preact: Signal<T>; |
There was a problem hiding this comment.
Is this "preact" the library providing the signals? What would this look like for lit of maybe react?
There was a problem hiding this comment.
Yes, but like you can set those words to whatever. Like, there's two phases to integrating with this. Declaring what type of signals you're dealing in:
import {Signal as PreactSignal} from 'preact blah blah';
import {Signal as AngularSignal} from '@angular/core';
import {Signal as AvaSignal} from '@ava-cassiopeia/fake-signal';
declare module '@a2ui/web_core/reactivity/signals' {
interface SignalKinds<T> {
preact: PreactSignal<T>;
angular: AngularSignal<T>;
coolestSignals: AvaSignal<T>;
}
}The key thing here is that we, as web core, don't define the types of signals ahead of time so that anyone can plug in any signal impl., even one we don't know about, as you can see with coolestSignals above.
Then, when they start constructing our core library objects, those objects will want for a FrameworkSignal implementation that uses one of those signals, a la:
export const AngularFrameworkSignal: FrameworkSignal<'angular'> = { /* impl */ };
export const AvaFrameworkSignal: FrameworkSignal<'coolestSignals'> = { /* impl */ };So in short, our library says something like:
constructor(readonly ctx: DataContext<'react'>) {}"I need a DataContext that specifically deals in React signals"
and then you might provide that by creating a DataContext that uses React signals:
const ctx = new DataContext(ReactFrameworkSignal);
// call the constructor from before. It won't take a DataContext that
// is using a non-matching FrameworkSignal implementation.
new FakeThing(ctx);
Reworks the generic signals impl that I created in #960 to have better type safety.
In this implementation, downstream libraries must explicitly provide a definition for their signals, and in return we'll preserve signal types as they transit into and out of web core. An example of how this works is in the test file, which has to do the same thing to be compliant with the code.
These changes came about as I started trying to wire these through the core library and was running into annoying typing issues - this should fix all that, and I'll issue a followup PR that actually uses these in web core.