Skip to content

Commit

Permalink
refactor: simplify/improve effect types
Browse files Browse the repository at this point in the history 
I will refer to all of `create(Computed|RenderEffect|Effect|Memo)` as `createEffect` for brevity.
- Simplified `createEffect` such that it no longer needs the use of rest params or extra generics (for `createMemo`).
- Added a default type `any` in `createEffect<Next = any, ...>` to replicate the default typescript behaviour of providing `any` as a fallback when parameters are not explicitly typed and cannot be inferred. This should feel more familiar than falling back to `unknown`. This "fixes" certain test cases where the `createEffect` parameter was inferred as `unknown`.
- Renamed the generic types of `Computation` to be consistent with other generic types containing `Prev, Next` generics (was confusingly `Init, Next` even though `Init` was used as `Prev` would be used).
- Corrected instances of `Memo<Init, Next>` to `Memo<Init | Next, Next>`.
- Added test cases for failing partial inference (it isn't in typescript yet)
  • Loading branch information
@otonashixav
otonashixav committed Apr 2, 2022
1 parent 9cd3f26 commit 4564834
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 63 deletions.
100 changes: 49 additions & 51 deletions packages/solid/src/reactive/signal.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ export interface Owner {
componentName?: string;
}

export interface Computation<Init, Next extends Init = Init> extends Owner {
fn: EffectFunction<Init, Next>;
export interface Computation<Prev, Next extends Prev = Prev> extends Owner {
fn: EffectFunction<Prev, Next>;
state: number;
tState?: number;
sources: SignalState<Next>[] | null;
sources: SignalState<any>[] | null;
sourceSlots: number[] | null;
value?: Init;
value?: Prev;
updatedAt: number | null;
pure: boolean;
user?: boolean;
Expand Down Expand Up @@ -213,16 +213,17 @@ export type EffectFunction<Prev, Next extends Prev = Prev> = (v: Prev) => Next;
*
* @description https://www.solidjs.com/docs/latest/api#createcomputed
*/
export function createComputed<Next, Init = Next>(
// defaulting Next to any replicates the default typescript behaviour of typing params as any when not explicitly typed and not inferable
export function createComputed<Next = any, Init = never>(
fn: EffectFunction<undefined | Init | Next, Next>, // Init delays inference, could use NoInfer but this is easier to replicate
value?: undefined,
options?: EffectOptions
): void;
export function createComputed<Next = any, Init = Next>(
fn: EffectFunction<Init | Next, Next>,
value: Init,
options?: EffectOptions
): void;
export function createComputed<Next, Init = undefined>(
..._: undefined extends Init
? [fn: EffectFunction<Init | Next, Next>, value?: Init, options?: EffectOptions]
: [fn: EffectFunction<Init | Next, Next>, value: Init, options?: EffectOptions]
): void;
export function createComputed<Next, Init>(
fn: EffectFunction<Init | Next, Next>,
value: Init,
Expand All @@ -248,16 +249,17 @@ export function createComputed<Next, Init>(
*
* @description https://www.solidjs.com/docs/latest/api#createrendereffect
*/
export function createRenderEffect<Next, Init = Next>(
// defaulting Next to any replicates the default typescript behaviour of typing params as any when not explicitly typed and not inferable
export function createRenderEffect<Next = any, Init = never>(
fn: EffectFunction<undefined | Init | Next, Next>, // Init delays inference, could use NoInfer but this is easier to replicate
value?: undefined,
options?: EffectOptions
): void;
export function createRenderEffect<Next = any, Init = Next>(
fn: EffectFunction<Init | Next, Next>,
value: Init,
options?: EffectOptions
): void;
export function createRenderEffect<Next, Init = undefined>(
..._: undefined extends Init
? [fn: EffectFunction<Init | Next, Next>, value?: Init, options?: EffectOptions]
: [fn: EffectFunction<Init | Next, Next>, value: Init, options?: EffectOptions]
): void;
export function createRenderEffect<Next, Init>(
fn: EffectFunction<Init | Next, Next>,
value: Init,
Expand All @@ -283,17 +285,18 @@ export function createRenderEffect<Next, Init>(
*
* @description https://www.solidjs.com/docs/latest/api#createeffect
*/
export function createEffect<Next, Init = Next>(
// defaulting Next to any replicates the default typescript behaviour of typing params as any when not explicitly typed and not inferable
export function createEffect<Next = any, Init = never>(
fn: EffectFunction<undefined | Init | Next, Next>, // Init delays inference, could use NoInfer but this is easier to replicate
value?: undefined,
options?: EffectOptions
): void;
export function createEffect<Next = any, Init = Next>(
fn: EffectFunction<Init | Next, Next>,
value: Init,
options?: EffectOptions
): void;
export function createEffect<Next, Init = undefined>(
..._: undefined extends Init
? [fn: EffectFunction<Init | Next, Next>, value?: Init, options?: EffectOptions]
: [fn: EffectFunction<Init | Next, Next>, value: Init, options?: EffectOptions]
): void;
export function createEffect<Next, Init = Next>(
export function createEffect<Next, Init>(
fn: EffectFunction<Init | Next, Next>,
value: Init,
options?: EffectOptions
Expand Down Expand Up @@ -340,8 +343,8 @@ export function createReaction(onInvalidate: () => void, options?: EffectOptions
};
}

interface Memo<Prev, Next = Prev> extends SignalState<Next>, Computation<Next> {
tOwned?: Computation<Prev | Next, Next>[];
interface Memo<Prev, Next extends Prev = Prev> extends SignalState<Next>, Computation<Next> {
tOwned?: Computation<Prev, Next>[];
}

export interface MemoOptions<T> extends EffectOptions {
Expand All @@ -363,43 +366,37 @@ export interface MemoOptions<T> extends EffectOptions {
*
* @description https://www.solidjs.com/docs/latest/api#creatememo
*/
// The extra _Next generic parameter separates inference of the effect input
// parameter type from inference of the effect return type, so that the effect
// return type is always used as the memo Accessor's return type.
export function createMemo<Next extends _Next, Init = Next, _Next = Next>(
fn: EffectFunction<Init | _Next, Next>,
value: Init,
// defaulting Next to any replicates the default typescript behaviour of typing params as any when not explicitly typed and not inferable
export function createMemo<Next = any, Init = never>(
fn: EffectFunction<undefined | Init | Next, Next>, // Init delays inference, could use NoInfer but this is easier to replicate
value?: undefined,
options?: MemoOptions<Next>
): Accessor<Next>;
export function createMemo<Next extends _Next, Init = undefined, _Next = Next>(
..._: undefined extends Init
? [fn: EffectFunction<Init | _Next, Next>, value?: Init, options?: MemoOptions<Next>]
: [fn: EffectFunction<Init | _Next, Next>, value: Init, options?: MemoOptions<Next>]
export function createMemo<Next = any, Init = Next>(
fn: EffectFunction<Init | Next, Next>,
value: Init,
options?: MemoOptions<Next>
): Accessor<Next>;
export function createMemo<Next extends _Next, Init, _Next>(
fn: EffectFunction<Init | _Next, Next>,
export function createMemo<Next, Init>(
fn: EffectFunction<Init | Next, Next>,
value: Init,
options?: MemoOptions<Next>
): Accessor<Next> {
options = options ? Object.assign({}, signalOptions, options) : signalOptions;

const c: Partial<Memo<Init, Next>> = createComputation(
fn,
value,
true,
0,
"_SOLID_DEV_" ? options : undefined
) as Partial<Memo<Init, Next>>;
const c = createComputation(fn, value, true, 0, "_SOLID_DEV_" ? options : undefined) as Partial<
Memo<Init | Next, Next>
>;

c.pending = NOTPENDING;
c.observers = null;
c.observerSlots = null;
c.comparator = options.equals || undefined;
if (Scheduler && Transition && Transition.running) {
c.tState = STALE;
Updates!.push(c as Memo<Init, Next>);
} else updateComputation(c as Memo<Init, Next>);
return readSignal.bind(c as Memo<Init, Next>);
Updates!.push(c as Memo<Init | Next, Next>);
} else updateComputation(c as Memo<Init | Next, Next>);
return readSignal.bind(c as Memo<Init | Next, Next>);
}

export interface Resource<T> extends Accessor<T> {
Expand Down Expand Up @@ -1306,9 +1303,10 @@ function createComputation<Next, Init = unknown>(
"computations created outside a `createRoot` or `render` will never be disposed"
);
else if (Owner !== UNOWNED) {
if (Transition && Transition.running && (Owner as Memo<Init, Next>).pure) {
if (!(Owner as Memo<Init, Next>).tOwned) (Owner as Memo<Init, Next>).tOwned = [c];
else (Owner as Memo<Init, Next>).tOwned!.push(c);
if (Transition && Transition.running && (Owner as Memo<Init | Next, Next>).pure) {
if (!(Owner as Memo<Init | Next, Next>).tOwned)
(Owner as Memo<Init | Next, Next>).tOwned = [c];
else (Owner as Memo<Init | Next, Next>).tOwned!.push(c);
} else {
if (!Owner.owned) Owner.owned = [c];
else Owner.owned.push(c);
Expand All @@ -1317,7 +1315,7 @@ function createComputation<Next, Init = unknown>(
c.name =
(options && options.name) ||
`${(Owner as Computation<any>).name || "c"}-${
(Owner.owned || (Owner as Memo<Init, Next>).tOwned!).length
(Owner.owned || (Owner as Memo<Init | Next, Next>).tOwned!).length
}`;
}

Expand Down
25 changes: 13 additions & 12 deletions packages/solid/test/signals.type-tests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ createEffect(() => {
}, "init");

createEffect(prev => {
// @ts-expect-error FIXME prev is inferred as unknown, so not assignable to string|number. Can we make it inferred?
const p: string = prev;
return p + "hello";
}, "init");
Expand All @@ -40,7 +39,6 @@ createEffect(() => {
}, 123);

createEffect(prev => {
// @ts-expect-error FIXME prev is inferred as unknown, so not assignable to string|number. Can we make it inferred?
const p: string = prev;
return p + "hello";
}, 123);
Expand Down Expand Up @@ -158,7 +156,6 @@ createComputed(() => {
}, "init");

createComputed(prev => {
// @ts-expect-error FIXME prev is inferred as unknown, so not assignable to string|number. Can we make it inferred?
const p: string = prev;
return p + "hello";
}, "init");
Expand All @@ -173,7 +170,6 @@ createComputed(() => {
}, 123);

createComputed(prev => {
// @ts-expect-error FIXME prev is inferred as unknown, so not assignable to string|number. Can we make it inferred?
const p: string = prev;
return p + "hello";
}, 123);
Expand All @@ -199,7 +195,7 @@ createComputed((num: number | undefined): number | undefined => 123);

createComputed((num?: number): number | undefined => 123);

createComputed<number>((v: number | string): number => 123, 123);
createComputed<number>(v => 123, 123);
createComputed<number | string>((v: number | string): number => 123, 123);

// @ts-expect-error undefined initial value not assignable to input parameter
Expand Down Expand Up @@ -291,7 +287,6 @@ createRenderEffect(() => {
}, "init");

createRenderEffect(prev => {
// @ts-expect-error FIXME prev is inferred as unknown, so not assignable to string|number. Can we make it inferred?
const p: string = prev;
return p + "hello";
}, "init");
Expand All @@ -306,7 +301,6 @@ createRenderEffect(() => {
}, 123);

createRenderEffect(prev => {
// @ts-expect-error FIXME prev is inferred as unknown, so not assignable to string|number. Can we make it inferred?
const p: string = prev;
return p + "hello";
}, 123);
Expand Down Expand Up @@ -463,7 +457,6 @@ const mv0 = createMemo(() => {
const mv0t: string = mv0();

const mv1 = createMemo(prev => {
// @ts-expect-error FIXME prev is inferred as unknown, so not assignable to string|number. Can we make it inferred?
const p: string = prev;
return p + "hello";
}, "init");
Expand All @@ -481,7 +474,6 @@ const mv2 = createMemo(() => {
const mv2t: string = mv2();

const mv3 = createMemo(prev => {
// @ts-expect-error FIXME prev is inferred as unknown, so not assignable to string|number. Can we make it inferred?
const p: string | number = prev;
return p + "hello";
}, 123);
Expand Down Expand Up @@ -580,7 +572,7 @@ const m2: Accessor<number | undefined> = createMemo(() => 123);
const m3: //
Accessor<undefined> = createMemo(() => {});
const m4: Accessor<void> = createMemo(() => {});
const m5: Accessor<number | undefined> = createMemo(
const m5: Accessor<void> = createMemo(
// @ts-expect-error void can't be assigned to anything!
(v?: number) => {}
);
Expand All @@ -602,7 +594,7 @@ const m11: Accessor<number | undefined> = createMemo<number | undefined>(
v => {},
123
);
const m12: Accessor<number | undefined> = createMemo(
const m12: Accessor<void> = createMemo(
// @ts-expect-error void can't be assigned to anything!
(v?: number) => {},
undefined
Expand All @@ -625,7 +617,6 @@ const m18: Accessor<number> =
const m19: Accessor<number> =
// @ts-expect-error undefined initial value is not assignable to the number parameter
createMemo((v: number | string): number => 123);
// @ts-expect-error because the number return cannot be assigned to the boolean|string parameter
const m20: Accessor<number> =
// @ts-expect-error because the number return cannot be assigned to the boolean|string parameter
createMemo((v: boolean | string): number => 123);
Expand Down Expand Up @@ -919,3 +910,13 @@ createRenderEffect<number | boolean>(
// @ts-expect-error string return is not assignable to number|boolean
"foo"
);

// FIXME cases failing due to partial generic inference not being implemented
// @ts-expect-error second generic is not inferred and remains as number
const a7: Accessor<number> = createMemo<number>((v: number | string) => 123, "asd");
// @ts-expect-error second generic is not inferred and remains as number
createEffect<number>((v: number | string) => 123, "asd");
// @ts-expect-error second generic is not inferred and remains as number
createComputed<number>((v: number | string) => 123, "asd");
// @ts-expect-error second generic is not inferred and remains as number
createRenderEffect<number>((v: number | string) => 123, "asd");

0 comments on commit 4564834

Please sign in to comment.