Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions renderers/web_core/src/v0_9/reactivity/signals-testing.shared.ts
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);
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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');
    });

});
}
59 changes: 59 additions & 0 deletions renderers/web_core/src/v0_9/reactivity/signals.angular.test.ts
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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();
  },

} 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);
45 changes: 45 additions & 0 deletions renderers/web_core/src/v0_9/reactivity/signals.preact.test.ts
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),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
  })

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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!

149 changes: 0 additions & 149 deletions renderers/web_core/src/v0_9/reactivity/signals.test.ts

This file was deleted.

38 changes: 23 additions & 15 deletions renderers/web_core/src/v0_9/reactivity/signals.ts
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -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
Copy link
Copy Markdown
Collaborator Author

@ditman ditman Mar 31, 2026

Choose a reason for hiding this comment

The 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 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!).


/**
* 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.
Expand All @@ -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;
}
Loading