Fix #466: Type of setState argument should be Partial<StateType> #467

mseddon · 2016-12-21T14:23:14Z

No description provided.

coveralls · 2016-12-21T14:27:04Z

Coverage remained the same at 97.513% when pulling 386bb3a on mseddon:setstate-typing into 75acb15 on developit:master.

developit · 2017-01-04T07:32:36Z

/cc @kruczy @jrf0110 who know a bunch about tsd

coveralls · 2017-01-04T07:37:14Z

Coverage remained the same at 97.522% when pulling 9fbcdb1 on mseddon:setstate-typing into 074aa7a on developit:master.

jrf0110 · 2017-01-04T15:57:21Z

src/preact.d.ts

@@ -52,7 +52,7 @@ declare namespace preact {

 		linkState:(name:string) => (event: Event) => void;

-		setState(state:StateType, opts?:any):void;
+		setState(state:Partial<StateType>, opts?:any):void;


This is cool. I've been making most of my State prop declarations optional, which is exactly what that generic type does https://github.com/Microsoft/TypeScript/pull/12276/files#diff-a6b488d9bd802977827b535a3011c1f3R1350

My only issue with using this is that it requires typescript@next if I'm not mistaken, and this could break some folks projects if released. I'm personally using typescript@next, so I'm good :)

[email protected] supports this, but yeah, it's a bit bleeding edge. :)

Awesome. Using Partial is the way to go here. Just wish there was a way for libraries to specify what version of TypeScript their definition files require to compile.

coveralls · 2017-01-05T14:46:50Z

Changes Unknown when pulling c08c7e2 on mseddon:setstate-typing into ** on developit:master**.

robertknight · 2017-01-05T22:24:49Z

I've just done some testing with TypeScript 2.0 and it looks like it is quite easy for users of earlier versions of TypeScript to add a "polyfill" for Partial<T> which would allow them to use these latest definitions:

type Partial<T> = T;

With this, any reference to Partial<T> is treated as if it were just T. On that basis, one option is that we could make this change but perhaps add a comment in the type definitions for the benefit of anyone using earlier TS versions, eg:

// Note: Partial<T> is a TS 2.1 feature. See <link to this PR>
setState(state: Partial<State>)

Thoughts?

SleeplessByte · 2017-01-13T18:14:29Z

@robertknight The problem with that "polyfill" is that it's not. Nothing would be optional. Instead aliasing as any would be closer even :(

I think saying that you need TS 2.1+ is fine. TS 2.0 to 2.1 does not introduce breaking changes, even we upgraded from 1.3 to 2.1 and it was fine. Everything was stashed away with flags anyway.

robertknight · 2017-01-13T19:15:04Z

@robertknight The problem with that "polyfill" is that it's not.
Nothing would be optional. Instead aliasing as any would be closer even :(

That's why I put "polyfill" in quotes. It would then work the same as it does now using the current setState typings, so there would be no regression. Once users update to TS 2.1 they will be able to take advantage of the proper feature.

So to update the status here, the current recommendation from the TypeScript team based on the discussion for the React typings is that there should be a grace period after a new TS release before releasing typings updates that depend on the new release.

I would suggest that we track DefinitelyTyped/DefinitelyTyped#13155 and once that is merged then we can do the same here. That way Preact and React typings will be in sync.

SleeplessByte · 2017-01-13T19:31:01Z

Then yup. I completely agree.

…

On Fri, 13 Jan 2017, 20:15 Robert Knight, ***@***.***> wrote: @robertknight <https://github.com/robertknight> The problem with that "polyfill" is that it's not. Nothing would be optional. Instead aliasing as any would be closer even :( That's why I put "polyfill" in quotes. It would then work the same as it does now using the current setState typings, so there would be no regression. Once users update to TS 2.1 they will be able to take advantage of the proper feature. So to update the status here, the current recommendation from the TypeScript team based on the discussion for the React typings is that there should be a grace period after a new TS release before releasing typings updates that depend on the new release. I would suggest that we track DefinitelyTyped/DefinitelyTyped#13155 <DefinitelyTyped/DefinitelyTyped#13155> and once that is merged then we can do the same here. That way Preact and React typings will be in sync. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#467 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB35WDbkQONjrDf7rGAmk8nQEvTp_Dntks5rR807gaJpZM4LS_xP> .

robertknight · 2017-01-24T08:54:46Z

The corresponding PR for React has been merged upstream. setState in the React definitions uses Pick rather than Partial: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/99d41f818c2de87790ce1f02956cbc39bce40f40/react/index.d.ts#L169

coveralls · 2017-01-24T08:57:15Z

Coverage remained the same at 97.695% when pulling d9a0755 on mseddon:setstate-typing into 472cb2b on developit:master.

SleeplessByte · 2017-01-24T14:36:50Z

@robertknight yes. The dropped "optional key" support in state and chose pick.

DefinitelyTyped/DefinitelyTyped#13155 based of microsoft/TypeScript#12793 (comment)

Preact could follow this.

vutran · 2017-01-24T17:27:01Z

src/preact.d.ts

@@ -45,7 +45,7 @@ declare namespace preact {

 		linkState:(name:string) => (event: Event) => void;

-		setState(state:StateType, callback?:() => void):void;
+		setState(state:Partial<StateType>, callback?:() => void):void;
 		setState(fn:(prevState:StateType, props:PropsType) => StateType, callback?:() => void):void;


Should be able to return a partial state here as well.

mseddon · 2017-01-25T09:59:30Z

Updated to use Pick, and added the other signature.

coveralls · 2017-01-25T10:02:20Z

Coverage remained the same at 97.695% when pulling e063e2b on mseddon:setstate-typing into 472cb2b on developit:master.

coveralls · 2017-01-29T18:12:28Z

Coverage remained the same at 97.695% when pulling a7d85f4 on mseddon:setstate-typing into 76fcbee on developit:master.

robertknight

I've just given this a quick test with TypeScript 2.1.5. Works nicely. Thanks @mseddon !

mseddon · 2017-01-30T18:04:54Z

🎉

developit · 2017-01-30T23:00:08Z

Awesome, so happy to have this merged :D

robertknight mentioned this pull request Dec 28, 2016

setState typescript signature is too restrictive. #466

Closed

developit requested a review from robertknight January 4, 2017 07:31

jrf0110 reviewed Jan 4, 2017

View reviewed changes

vutran reviewed Jan 24, 2017

View reviewed changes

mseddon force-pushed the setstate-typing branch from d9a0755 to 37c066f Compare January 25, 2017 09:51

Fix preactjs#466: Type of setState argument should be Pick<StateType, K>

e063e2b

mseddon force-pushed the setstate-typing branch from 37c066f to e063e2b Compare January 25, 2017 09:58

Merge branch 'master' into setstate-typing

a7d85f4

robertknight approved these changes Jan 29, 2017

View reviewed changes

robertknight merged commit 8614396 into preactjs:master Jan 29, 2017

Uh oh!

Fix #466: Type of setState argument should be Partial<StateType> #467

Fix #466: Type of setState argument should be Partial<StateType> #467

Uh oh!

Conversation

mseddon commented Dec 21, 2016

Uh oh!

coveralls commented Dec 21, 2016

Uh oh!

developit commented Jan 4, 2017

Uh oh!

coveralls commented Jan 4, 2017

Uh oh!

jrf0110 Jan 4, 2017

Choose a reason for hiding this comment

Uh oh!

mseddon Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

jrf0110 Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 5, 2017

Uh oh!

robertknight commented Jan 5, 2017

Uh oh!

SleeplessByte commented Jan 13, 2017

Uh oh!

robertknight commented Jan 13, 2017

Uh oh!

SleeplessByte commented Jan 13, 2017 via email

Uh oh!

robertknight commented Jan 24, 2017

Uh oh!

coveralls commented Jan 24, 2017

Uh oh!

SleeplessByte commented Jan 24, 2017

Uh oh!

vutran Jan 24, 2017

Choose a reason for hiding this comment

Uh oh!

mseddon commented Jan 25, 2017

Uh oh!

coveralls commented Jan 25, 2017

Uh oh!

coveralls commented Jan 29, 2017

Uh oh!

robertknight left a comment

Choose a reason for hiding this comment

Uh oh!

mseddon commented Jan 30, 2017

Uh oh!

developit commented Jan 30, 2017

Uh oh!

Uh oh!