Skip to content

Commit

Permalink
fix(core): add toString implementation to signals (#54002)
Browse files Browse the repository at this point in the history
Since signals are function, currently stringifying them reveals the implementation of the function. This can lead to confusion since it contains internal implementation details. These changes add static `toString` function to address the issue.

**Note:** it's tempting to have `toString` output the actual value of the signal, but that would encourage users not to call the function which will be problematic in the long run. That's why these changes are using a static string instead.

PR Close #54002
  • Loading branch information
crisbeto authored and thePunderWoman committed Jan 25, 2024
1 parent 75aeae4 commit 656bc28
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 0 deletions.
4 changes: 4 additions & 0 deletions packages/core/primitives/signals/src/computed.ts
Expand Up @@ -40,6 +40,9 @@ export type ComputedGetter<T> = (() => T)&{
[SIGNAL]: ComputedNode<T>;
};

/** Function used as the `toString` implementation of computed. */
const computedToString = () => '[COMPUTED]';

/**
* Create a computed signal which derives a reactive value from an expression.
*/
Expand All @@ -61,6 +64,7 @@ export function createComputed<T>(computation: () => T): ComputedGetter<T> {
return node.value;
};
(computed as ComputedGetter<T>)[SIGNAL] = node;
computed.toString = computedToString;
return computed as unknown as ComputedGetter<T>;
}

Expand Down
4 changes: 4 additions & 0 deletions packages/core/primitives/signals/src/signal.ts
Expand Up @@ -35,6 +35,9 @@ export interface SignalGetter<T> extends SignalBaseGetter<T> {
readonly[SIGNAL]: SignalNode<T>;
}

/** Function used as the `toString` implementation of signals. */
const signalToString = () => '[SIGNAL]';

/**
* Create a `Signal` that can be set or updated directly.
*/
Expand All @@ -46,6 +49,7 @@ export function createSignal<T>(initialValue: T): SignalGetter<T> {
return node.value;
}) as SignalGetter<T>;
(getter as any)[SIGNAL] = node;
getter.toString = signalToString;
return getter;
}

Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/authoring/input_signal.ts
Expand Up @@ -68,6 +68,9 @@ export interface InputSignal<ReadT, WriteT = ReadT> extends Signal<ReadT> {
[ɵINPUT_SIGNAL_BRAND_WRITE_TYPE]: WriteT;
}

/** Function used as the `toString` implementation of input signals. */
const signalInputToString = () => '[INPUT_SIGNAL]';

/**
* Creates an input signal.
*
Expand Down Expand Up @@ -99,5 +102,6 @@ export function createInputSignal<ReadT, WriteT>(
}

(inputValueFn as any)[SIGNAL] = node;
inputValueFn.toString = signalInputToString;
return inputValueFn as InputSignal<ReadT, WriteT>;
}
4 changes: 4 additions & 0 deletions packages/core/src/render3/query_reactive.ts
Expand Up @@ -18,6 +18,9 @@ import {collectQueryResults, getTQuery, loadQueryInternal, materializeViewResult
import {Signal} from './reactivity/api';
import {getLView} from './state';

/** Function used as the `toString` implementation of query signals. */
const querySignalToString = () => '[QUERY_SIGNAL]';

function createQuerySignalFn<V>(firstOnly: true, required: true): Signal<V>;
function createQuerySignalFn<V>(firstOnly: true, required: false): Signal<V|undefined>;
function createQuerySignalFn<V>(firstOnly: false, required: false): Signal<ReadonlyArray<V>>;
Expand Down Expand Up @@ -45,6 +48,7 @@ function createQuerySignalFn<V>(firstOnly: boolean, required: boolean) {
}
}
(signalFn as any)[SIGNAL] = node;
signalFn.toString = querySignalToString;

return signalFn;
}
Expand Down
5 changes: 5 additions & 0 deletions packages/core/test/authoring/input_signal_spec.ts
Expand Up @@ -79,4 +79,9 @@ describe('input signal', () => {
node.applyValueToInputSignal(node, 1);
expect(expr()).toBe(1001);
});

it('should have a toString implementation', () => {
const signal = input(0);
expect(signal + '').toBe('[INPUT_SIGNAL]');
});
});
6 changes: 6 additions & 0 deletions packages/core/test/signals/computed_spec.ts
Expand Up @@ -186,4 +186,10 @@ describe('computed', () => {

expect(illegal).toThrow();
});

it('should have a toString implementation', () => {
const counter = signal(0);
const double = computed(() => counter() * 2);
expect(double + '').toBe('[COMPUTED]');
});
});
5 changes: 5 additions & 0 deletions packages/core/test/signals/signal_spec.ts
Expand Up @@ -123,6 +123,11 @@ describe('signals', () => {
expect(double()).toBe(4);
});

it('should have a toString implementation', () => {
const state = signal(false);
expect(state + '').toBe('[SIGNAL]');
});

describe('optimizations', () => {
it('should not repeatedly poll status of a non-live node if no signals have changed', () => {
const unrelated = signal(0);
Expand Down

0 comments on commit 656bc28

Please sign in to comment.