Skip to content

Commit

Permalink
Fix some inaccurate typings and Selector bug (#327)
Browse files Browse the repository at this point in the history
* Fix incorrect argument type of createEffect

When an initial value is passed to createComputed, createRenderEffect or
createEffect, and the return type of the passed function doesn't
include undefined, the argument will never be undefined either.
Additionally, when no initial value is given, there is no reason to
restrict the return type to a defined value. The code can handle
undefined values just fine.

Some examples that fail to typecheck, but work in practice have been
added as test cases.

This commit adds overloads to the mentioned functions to remove
undefined from the argument type when an initial value is given, and
allow undefined in the return type otherwise.

* Fix types of functions which can return undefined

The setter returned by `createSignal()` accepts an optional argument,
but returns a non-optional type. This commit adds a test case where this
results in incorrectly typing an undefined value as defined.

Similarly, `createContext()` creates a context with a default value of
undefined. However, when used in `useContext()`, it will return a
non-undefined type. This is possible because `lookup()` returns `any`.
Typescript incorrectly assumed that `any` equals `T` and forgets that it
could also be undefined. This commit works around that by encoding the
possible undefined in the `T` stored in `Context<>`.

* Fix selector not working when passing falsy value

The primary use for selectors is using an array index as key. This means
that the value `0` is likely to be used quite often. However, due to the
use of a falsy check instead of an undefined check, passing 0 will not
cause an update when the key is unset. This is demonstrated by a test.
This commit replaces the falsy check with an undefined check.
  • Loading branch information
Jager567 authored Feb 2, 2021
1 parent cdfeab1 commit 851bd63
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 5 deletions.
16 changes: 12 additions & 4 deletions packages/solid/src/reactive/signal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export function createRoot<T>(fn: (dispose: () => void) => T, detachedOwner?: Ow
return result!;
}

export function createSignal<T>(): [() => T | undefined, (v?: T) => T];
export function createSignal<T>(): [() => T | undefined, <U extends T | undefined>(v?: U) => U];
export function createSignal<T>(
value: T,
areEqual?: boolean | ((prev: T, next: T) => boolean),
Expand All @@ -120,14 +120,20 @@ export function createSignal<T>(
return [readSignal.bind(s), writeSignal.bind(s)];
}

export function createComputed<T>(fn: (v: T) => T, value: T): void;
export function createComputed<T>(fn: (v?: T) => T | undefined): void;
export function createComputed<T>(fn: (v?: T) => T, value?: T): void {
updateComputation(createComputation(fn, value, true));
}

export function createRenderEffect<T>(fn: (v: T) => T, value: T): void;
export function createRenderEffect<T>(fn: (v?: T) => T | undefined): void;
export function createRenderEffect<T>(fn: (v?: T) => T, value?: T): void {
updateComputation(createComputation(fn, value, false));
}

export function createEffect<T>(fn: (v: T) => T, value: T): void;
export function createEffect<T>(fn: (v?: T) => T | undefined): void;
export function createEffect<T>(fn: (v?: T) => T, value?: T): void {
if (globalThis._$HYDRATION && globalThis._$HYDRATION.asyncSSR) return;
runEffects = runUserEffects;
Expand Down Expand Up @@ -199,7 +205,7 @@ export function createSelector<T, U>(
(p: T | undefined) => {
const v = source();
for (const key of subs.keys())
if (fn(key, v) || (p && fn(key, p))) {
if (fn(key, v) || (p !== undefined && fn(key, p))) {
const c = subs.get(key)!;
c.state = STALE;
if (c.pure) Updates!.push(c);
Expand Down Expand Up @@ -367,10 +373,12 @@ export function serializeGraph(owner?: Owner | null): GraphRecord {
export interface Context<T> {
id: symbol;
Provider: (props: { value: T; children: any }) => any;
defaultValue?: T;
defaultValue: T;
}

export function createContext<T>(defaultValue?: T): Context<T> {
export function createContext<T>(): Context<T | undefined>
export function createContext<T>(defaultValue: T): Context<T>
export function createContext<T>(defaultValue?: T): Context<T | undefined> {
const id = Symbol("context");
return { id, Provider: createProvider(id), defaultValue };
}
Expand Down
94 changes: 93 additions & 1 deletion packages/solid/test/signals.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
createRoot,
createSignal,
createEffect,
createRenderEffect,
createComputed,
createDeferred,
createMemo,
Expand All @@ -10,7 +11,9 @@ import {
on,
onMount,
onCleanup,
onError
onError,
createContext,
useContext
} from "../src";

describe("Create signals", () => {
Expand Down Expand Up @@ -111,6 +114,17 @@ describe("Update signals", () => {
});
});
});
test("Set signal returns argument", () => {
const [_, setValue] = createSignal<number>();
const res1: undefined = setValue(undefined);
expect(res1).toBe(undefined);
const res2: number = setValue(12);
expect(res2).toBe(12);
const res3 = setValue(Math.random() >= 0 ? 12 : undefined);
expect(res3).toBe(12);
const res4 = setValue();
expect(res4).toBe(undefined);
});
});

describe("Untrack signals", () => {
Expand All @@ -129,6 +143,49 @@ describe("Untrack signals", () => {
});
});

describe("Typecheck computed and effects", () => {
test("No default value can return undefined", () => {
createRoot(() => {
let count = 0;
const [sign, setSign] = createSignal("thoughts");
const fn = (arg?: number) => {
count++;
sign();
expect(arg).toBe(undefined);
return arg;
};
createComputed(fn);
createRenderEffect(fn);
createEffect(fn);
setTimeout(() => {
expect(count).toBe(3);
setSign("update");
expect(count).toBe(6);
});
});
});
test("Default value never receives undefined", () => {
createRoot(() => {
let count = 0;
const [sign, setSign] = createSignal("thoughts");
const fn = (arg: number) => {
count++;
sign();
expect(arg).toBe(12);
return arg;
};
createComputed(fn, 12);
createRenderEffect(fn, 12);
createEffect(fn, 12);
setTimeout(() => {
expect(count).toBe(3);
setSign("update");
expect(count).toBe(6);
});
});
});
});

describe("onCleanup", () => {
test("Clean an effect", done => {
createRoot(() => {
Expand Down Expand Up @@ -318,4 +375,39 @@ describe("createSelector", () => {
});
});
});

test("zero index", done => {
createRoot(() => {
const [s, set] = createSignal<number>(-1),
isSelected = createSelector<number, number>(s);
let count = 0;
const list = [
createMemo(() => {
count++;
return isSelected(0) ? "selected" : "no";
})
];
expect(count).toBe(1);
expect(list[0]()).toBe("no");
setTimeout(() => {
count = 0;
set(0);
expect(count).toBe(1);
expect(list[0]()).toBe("selected");
count = 0;
set(-1);
expect(count).toBe(1);
expect(list[0]()).toBe("no");
done();
});
});
});
});

describe("create and use context", () => {
test("createContext without arguments defaults to undefined", () => {
const context = createContext<number>()
const res = useContext(context);
expect(res).toBe<typeof res>(undefined)
})
})

0 comments on commit 851bd63

Please sign in to comment.