Ts 3.4 inference improvements #348
Conversation
| @@ -70,27 +71,47 @@ it("can update readonly state via standard api", () => { | |||
| // NOTE: only when the function type is inferred | |||
| it("can infer state type from default state", () => { | |||
| type State = {readonly a:number} | boolean | |||
| type Recipe = (base?: State | undefined) => State | |||
| type Recipe = (base?: State | boolean) => State | |||
There was a problem hiding this comment.
doh, that should just be State
| : T extends AtomicObject | ||
| : T extends object | ||
| ? {-readonly [K in keyof T]: Draft<T[K]>} | ||
| : T // mostly: unknown & any |
There was a problem hiding this comment.
And also primitives like number | string | boolean 😛 Or am I misunderstanding this comment?
There was a problem hiding this comment.
eh yes, had a never first there, and AtomicObject at the start, will remove the comment
| ): (state?: Immutable<T>, ...rest: Tail<Params>) => Produced<Immutable<T>, ReturnType<Recipe>> | ||
|
|
||
| /** Normal producer */ | ||
| <Base, D = Draft<Base>, Return = void>( |
There was a problem hiding this comment.
Separating Base and D allows things like: produce<readonly string[]>([], (draft: string[]) => ...
|
yes, this was based on that branch. Main problem there was that the rest arguments became optional in the returned producer function. Beyond that not too much changed, but I think the inference flow is slightly clearer this way, in which case we can better document certain best practices. |
| : ( | ||
| // The `base` argument is required when `Rest` is required. | ||
| base: Immutable<Base> | undefined, | ||
| ...rest: Rest |
There was a problem hiding this comment.
in practice, this ...rest: Rest always resolved into ...any[], as soon as Base is set (not inferred) by using produce<X>((draft, x: number) => {})
| @@ -274,3 +297,68 @@ it("works with generic parameters", () => { | |||
| let arr: ReadonlyArray<typeof val> = 0 as any | |||
| insert(arr, 0, val) | |||
| }) | |||
|
|
|||
There was a problem hiding this comment.
In these two test cases I tried to capture two real-life scenarios:
- Your generic state shape is defined as mutable
- The generic state shape is defined as immutable
Given those two scenarios (tests), the tests below verify a few things:
- draft is always mutable, regardless the base state
- if
state: State, then ideallyproduce(state, fn)andproduce(fn)(state)should result inStateagain
There was a problem hiding this comment.
Ok, I think in the latest push I've found the solution that satisfies all those constraints! In the latest, the curried producer has it's own generic argument, and basically, what goes in, goes out. That means: immutable types in, immutable types out. Mutable types in, mutable types out. However, the generic argument has to at least satisfy that it is at least the Immutable<T> where T is the first argument of the recipe.
I think this solves all cases quite neatly. The only downside is that the tests that use exactType on the curried function now fails, because the curried producer has a wider type than the recipe. But technically that is actually valid, as the following statement is correct:
produce((draft: { count: 0 }) => { draft.count++ }) ({ count: 7, messages: [] })
The above now neatly does produce a new state of type { count, messages } rather than just { count } which it would be, based on the recipe
There was a problem hiding this comment.
cc @aleclarson
mweststrate
force-pushed
the
ts-3.4-inference-improvements
branch
from
d762fc0
to
3644aa8
Compare
|
@aleclarson I think this PR is done. I think this should be released as minor, although typescript types change (and TS 3.4 is probably required), there are no semantic or api changes to the library itself. |
|
...or I just finish 3.0 and merge it in there :) |
|
Just my two cents: You could use the types versions system in package.json to make this a non-breaking change. (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-1.html#version-selection-with-typesversions) Otherwise, this would break a lot of people’s code. |
|
@jineshshah36 I mentioned that here. If we do that, we need to use |
|
@mweststrate I'll see if I can review this today! |
|
Would it be too much to keep two separate test files within versions in them like the types would be, and then, you only run the file with the matching version? |
|
@jineshshah36 That would require more maintenance than going with |
|
Closing this branch as it it is merged to |
Improved type inference a bit:
producehas improved. Especially the rest parameters are now better typed and has less edge cases (before at random places| booleanwas added to the type inference, and rest arguments remained optional`produce<State>(fn)no longer works, however,produce((x: Draft<State>))will lead to the correct inference@aleclarson feel free to shoot at it! I think especially for currying type inference is now better and a bit more predictable, but this change is also kinda breaking on how to type producers