Skip to content

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

Merged
ryansolid merged 1 commit intosolidjs:mainfrom
otonashixav:fix-setter-param-type
Oct 9, 2021
Merged

fix: signal setter callback form incorrect prev parameter type#682
ryansolid merged 1 commit intosolidjs:mainfrom
otonashixav:fix-setter-param-type

Conversation

@otonashixav
Copy link
Copy Markdown
Contributor

Previously the prev parameter would erroneously be the same as the return type of the callback, instead of any type the signal could hold; see TS playground.

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 8, 2021

Pull Request Test Coverage Report for Build 1320346031

  • 1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.908%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/solid/src/reactive/signal.ts 1 2 50.0%
Totals Coverage Status
Change from base Build 1319595723: 0.0%
Covered Lines: 1108
Relevant Lines: 1178

💛 - Coveralls

@otonashixav otonashixav force-pushed the fix-setter-param-type branch from fbc3483 to b8caf6e Compare October 8, 2021 12:44
? <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

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

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

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.

@ryansolid ryansolid merged commit 04e5d6f into solidjs:main Oct 9, 2021
@otonashixav otonashixav deleted the fix-setter-param-type branch October 11, 2021 18:46
lilybw pushed a commit to lilybw/solid that referenced this pull request Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants