Skip to content

fix: signal setter callback form incorrect prev parameter type #682

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.

Sign up for GitHub

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

Merged
ryansolid merged 1 commit into from
Oct 9, 2021
Merged

fix: signal setter callback form incorrect prev parameter type #682

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions packages/solid/src/reactive/signal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import type { JSX } from "../jsx";

export type Accessor<T> = () => T;
export type Setter<T> = undefined extends T
? <U extends T>(v?: (U extends Function ? never : U) | ((prev?: U) => U)) => U
: <U extends T>(v: (U extends Function ? never : U) | ((prev: U) => U)) => U;
? <U extends T>(v?: (U extends Function ? never : U) | ((prev?: T) => U)) => U
: <U extends T>(v: (U extends Function ? never : U) | ((prev: T) => U)) => U;
Copy link
Copy Markdown
Member

@rturnq rturnq Oct 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the whole setter function should also return T rather than U for the same reason as prev. As it is the return value will be narrowed to the new value only. Consider the following:

const [signal, setSignal] = createSignal(false) // boolean
let value = setSignal(true) // typeof value has been narrowed to true

value = false // Error: Type 'false' is not assignable to type 'true'

Copy link
Copy Markdown
Contributor Author

@otonashixav otonashixav Oct 9, 2021

Choose a reason for hiding this comment

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

This test wants the setter to return U though. I also disagree that it should return T for the same reason as prev should be type T; the type of prev can't be known by the callback whereas the setter will always return U when passed U or a callback that returns U.

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

Copy link
Copy Markdown
Member

@ryansolid ryansolid Oct 9, 2021

Choose a reason for hiding this comment

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

Sounds good.. want to move on the release tonight so going to merge and make the other change.

export const equalFn = <T>(a: T, b: T) => a === b;
export const $PROXY = Symbol("solid-proxy");
const signalOptions = { equals: equalFn };
Expand Down Expand Up @@ -166,8 +166,8 @@ export function createSignal<T>(
((value: T extends Function ? never : T | ((p?: T) => T)) => {
if (typeof value === "function") {
if (Transition && Transition.running && Transition.sources.has(s))
value = value(s.pending !== NOTPENDING ? (s.pending as T) : s.tValue);
else value = value(s.pending !== NOTPENDING ? (s.pending as T) : s.value);
value = value(s.pending !== NOTPENDING ? s.pending : s.tValue);
else value = value(s.pending !== NOTPENDING ? s.pending : s.value);
}
return writeSignal(s, value);
}) as Setter<T>
Expand Down