-
Notifications
You must be signed in to change notification settings - Fork 1k
Create signal implementations that satisfies FrameworkSignal. Simplify SignalKinds.
#1017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /** | ||
| * Copyright 2026 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import assert from 'node:assert'; | ||
| import {describe, it} from 'node:test'; | ||
| import {FrameworkSignal} from './signals.js'; | ||
|
|
||
| /** | ||
| * Shared verification tests for FrameworkSignal implementations. | ||
| */ | ||
| export function runFrameworkSignalTests(name: string, SignalImpl: FrameworkSignal) { | ||
| describe(`FrameworkSignal ${name}`, () => { | ||
| it('round trip wraps and unwraps successfully', () => { | ||
| const val = 'hello'; | ||
| const wrapped = SignalImpl.wrap(val); | ||
| assert.strictEqual(SignalImpl.unwrap(wrapped), val); | ||
| }); | ||
|
|
||
| it('handles updates well', () => { | ||
| const signal = SignalImpl.wrap('first'); | ||
| const computedVal = SignalImpl.computed(() => `prefix ${SignalImpl.unwrap(signal)}`); | ||
|
|
||
| assert.strictEqual(SignalImpl.unwrap(signal), 'first'); | ||
| assert.strictEqual(SignalImpl.unwrap(computedVal), 'prefix first'); | ||
|
|
||
| SignalImpl.set(signal, 'second'); | ||
|
|
||
| assert.strictEqual(SignalImpl.unwrap(signal), 'second'); | ||
| assert.strictEqual(SignalImpl.unwrap(computedVal), 'prefix second'); | ||
| }); | ||
|
|
||
| describe('.isSignal()', () => { | ||
| it('validates a signal', () => { | ||
| const val = 'hello'; | ||
| const wrapped = SignalImpl.wrap(val); | ||
| assert.ok(SignalImpl.isSignal(wrapped)); | ||
| }); | ||
|
|
||
| it('rejects a non-signal', () => { | ||
| assert.strictEqual(SignalImpl.isSignal('hello'), false); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /** | ||
| * Copyright 2026 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import { | ||
| signal, | ||
| computed, | ||
| isSignal, | ||
| effect, | ||
| Signal as NgSignal, | ||
| WritableSignal as NgWritableSignal, | ||
| } from '@angular/core'; | ||
|
|
||
| import {FrameworkSignal} from './signals.js'; | ||
| import {runFrameworkSignalTests} from './signals-testing.shared.js'; | ||
|
|
||
| declare module './signals.js' { | ||
| // Setup the appropriate types for Angular Signals | ||
| interface SignalKinds<T> { | ||
| // @ts-ignore : Suppress cross-compilation interface overlap | ||
| readonly: NgSignal<T>; | ||
| // @ts-ignore : Suppress cross-compilation interface overlap | ||
| writable: NgWritableSignal<T>; | ||
| } | ||
| } | ||
|
|
||
| // Test FrameworkSignal with Angular signals explicitly mapped over SignalKinds. | ||
| const AngularSignal = { | ||
| computed: <T>(fn: () => T) => computed(fn), | ||
| isSignal: (val: unknown): val is NgSignal<any> => isSignal(val), | ||
| wrap: <T>(val: T) => signal(val), | ||
| unwrap: <T>(val: NgSignal<T>) => val(), | ||
| set: <T>(sig: NgWritableSignal<T>, value: T) => sig.set(value), | ||
| effect: (fn: () => void, cleanupCallback: () => void) => { | ||
| const e = effect(cleanupRegisterFn => { | ||
| cleanupRegisterFn(cleanupCallback); | ||
| fn(); | ||
| }); | ||
| return () => e.destroy(); | ||
| }, | ||
|
Comment on lines
+46
to
+52
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Please update the implementation to handle an optional effect: (fn: () => void, cleanupCallback?: () => void) => {
const e = effect(cleanupRegisterFn => {
if (cleanupCallback) {
cleanupRegisterFn(cleanupCallback);
}
fn();
});
return () => e.destroy();
}, |
||
| } as unknown as FrameworkSignal; // Bypass Mono-compilation interface overlap | ||
| // The cast above is needed because tsc is merging all our test files together, | ||
| // and the SignalKinds interface is being declared multiple times, causing a | ||
| // type collision. Normally, the AngularSignal would `satisfies FrameworkSignal`, | ||
| // and the declaration of SignalKinds wouldn't need to suppress anything. | ||
|
|
||
| runFrameworkSignalTests('Angular implementation', AngularSignal); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| /** | ||
| * Copyright 2026 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import {computed, effect, Signal as PSignal} from '@preact/signals-core'; | ||
|
|
||
| import {FrameworkSignal} from './signals.js'; | ||
| import {runFrameworkSignalTests} from './signals-testing.shared.js'; | ||
|
|
||
| declare module './signals.js' { | ||
| interface SignalKinds<T> { | ||
| // @ts-ignore : Suppress cross-compilation interface overlap | ||
| readonly: PSignal<T>; | ||
| // @ts-ignore : Suppress cross-compilation interface overlap | ||
| writable: PSignal<T>; | ||
| } | ||
| } | ||
|
|
||
| // Test FrameworkSignal with Preact signals explicitly mapped over SignalKinds. | ||
| const PreactSignal = { | ||
| computed: <T>(fn: () => T) => computed(fn), | ||
| isSignal: (val: unknown): val is PSignal<any> => val instanceof PSignal, | ||
| wrap: <T>(val: T) => new PSignal(val), | ||
| unwrap: <T>(val: PSignal<T>) => val.value, | ||
| set: <T>(sig: PSignal<T>, value: T) => (sig.value = value), | ||
| effect: (fn: () => void) => effect(fn), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation of The effect: (fn: () => void, cleanupCallback?: () => void) => effect(() => {
fn();
return cleanupCallback;
})
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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?) |
||
| } as unknown as FrameworkSignal; // Cast bypasses Mono-compilation interface overlap | ||
| // The cast above is needed because tsc is merging all our test files together, | ||
| // and the SignalKinds interface is being declared multiple times, causing a | ||
| // type collision. Normally, the AngularSignal would `satisfies FrameworkSignal`, | ||
| // and the declaration of SignalKinds wouldn't need to suppress anything. | ||
|
|
||
| runFrameworkSignalTests('Preact implementation', PreactSignal); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
This file was deleted.
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for putting this alternative together - do you think we should present this to the core a2ui team and see what they think?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,37 +14,45 @@ | |
| * limitations under the License. | ||
| */ | ||
|
|
||
| // SignalKinds and WritableSignalKinds are declared in such a way that | ||
| // SignalKinds is declared in such a way that | ||
| // downstream library impls can dynamically provide their Signal implementations | ||
| // in a type-safe way. Usage downstream might look something like: | ||
| // | ||
| // declare module '../reactivity/signals' { | ||
| // interface SignalKinds<T> { | ||
| // preact: Signal<T>; | ||
| // } | ||
| // interface WritableSignalKinds<T> { | ||
| // preact: Signal<T>; | ||
| // readonly: Signal<T>; | ||
| // writable: Signal<T>; | ||
| // } | ||
| // } | ||
| // | ||
| // This <T>, while unused, is required to pass through to a given Signal impl. | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| export interface SignalKinds<T> {} | ||
| export interface SignalKinds<T> { | ||
| _phantom?: T; | ||
| } | ||
|
|
||
| // This <T>, while unused, is required to pass through to a given Signal impl. | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| export interface WritableSignalKinds<T> {} | ||
| /** | ||
| * A generic representation of a read-only Signal. | ||
| * Resolves to the specific framework's signal type if augmented. | ||
| */ | ||
| export type Signal<T> = SignalKinds<T> extends { readonly: infer R } ? R : unknown; | ||
|
|
||
| /** | ||
| * A generic representation of a writable Signal. | ||
| * Resolves to the specific framework's signal type if augmented. | ||
| */ | ||
| export type WritableSignal<T> = SignalKinds<T> extends { writable: infer W } ? W : unknown; | ||
|
Comment on lines
+34
to
+44
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a usability problem. The way these two types are inferred is by extracting the "readonly" and "writable" properties from the HOWEVER, the 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 |
||
|
|
||
| /** | ||
| * A generic representation of a Signal that could come from any framework. | ||
| * For any library building on top of A2UI's web core lib, this must be | ||
| * implemented for their associated signals implementation. | ||
| */ | ||
| export interface FrameworkSignal<K extends keyof SignalKinds<any>> { | ||
| export interface FrameworkSignal { | ||
| /** | ||
| * Create a computed signal for this framework. | ||
| */ | ||
| computed<T>(fn: () => T): SignalKinds<T>[K]; | ||
| computed<T>(fn: () => T): Signal<T>; | ||
|
|
||
| /** | ||
| * Run a reactive effect. | ||
|
|
@@ -54,20 +62,20 @@ export interface FrameworkSignal<K extends keyof SignalKinds<any>> { | |
| /** | ||
| * Check if an arbitrary object is a framework signal. | ||
| */ | ||
| isSignal(val: unknown): val is SignalKinds<any>[K]; | ||
| isSignal(val: unknown): val is Signal<any>; | ||
|
|
||
| /** | ||
| * Wrap the value in a signal. | ||
| */ | ||
| wrap<T>(val: T): WritableSignalKinds<T>[K]; | ||
| wrap<T>(val: T): WritableSignal<T>; | ||
|
|
||
| /** | ||
| * Extract the value from a signal. | ||
| */ | ||
| unwrap<T>(val: SignalKinds<T>[K]): T; | ||
| unwrap<T>(val: Signal<T>): T; | ||
|
|
||
| /** | ||
| * Sets the value of the provided framework signal. | ||
| */ | ||
| set<T>(signal: WritableSignalKinds<T>[K], value: T): void; | ||
| set<T>(signal: WritableSignal<T>, value: T): void; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shared tests for
FrameworkSignalimplementations are missing coverage for theeffectfunction. According to the repository's style guide, code changes should have tests. Since theeffectfunction is a key part of theFrameworkSignalinterface, it should be tested to ensure all implementations behave correctly, especially regarding cleanup logic.I suggest adding a test case for
effect.