Skip to content
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.

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

Version 1.9.1 produce Typescript type inference is partly broken #270

Closed
benneq opened this issue Dec 16, 2018 · 10 comments
Closed

Version 1.9.1 produce Typescript type inference is partly broken #270

benneq opened this issue Dec 16, 2018 · 10 comments
Assignees

Comments

@benneq
Copy link

benneq commented Dec 16, 2018

Sample Code from readme.md:

const baseState = [
    {
        todo: "Learn typescript",
        done: true
    },
    {
        todo: "Try immer",
        done: false
    }
]

const nextState = produce(baseState, draftState => {
    draftState.push({todo: "Tweet about it", done: false })
    draftState[1].done = true
})

Type of nextState is void. It works correctly when I add return draftState at the end. Is this intended?

In my opinion there's no(?) case where produce shouldn't return anything. In other words: produce should always return something. Or am I wrong?


EDIT: I'm not entirely sure, but I think the typings should look something like that:

Old:

<S = any, R = never>(
        currentState: S,
        recipe: (this: Draft<S>, draft: Draft<S>) => void | R,
        listener?: PatchListener
    ): R

New:

<S, R>(
        currentState: S,
        recipe: (this: Draft<S>, draft: Draft<S>) => R,
        listener?: PatchListener
    ): R extends void ? S : R
aleclarson added a commit that referenced this issue Dec 16, 2018
@aleclarson aleclarson added the bug label Dec 16, 2018
@aleclarson
Copy link
Member

@benneq Yeah, your fix is spot on. The fix will be out in a sec.

@benneq
Copy link
Author

benneq commented Dec 16, 2018

Ah, I see, you were faster than me figuring this out :D

@aleclarson
Copy link
Member

aleclarson commented Dec 16, 2018

Oh actually, you need to do void extends R not R extends void, because otherwise it passes when R = number | void.

edit: Wait.. nevermind.

@benneq
Copy link
Author

benneq commented Dec 16, 2018

Thank you. That's really good to know :)

@aleclarson
Copy link
Member

Yeah it's one of those rites of passage when wrapping your head around Typescript. 😆

aleclarson added a commit that referenced this issue Dec 16, 2018
aleclarson added a commit that referenced this issue Dec 16, 2018
@aleclarson
Copy link
Member

Thanks for the bug report! ❤️

@benneq
Copy link
Author

benneq commented Dec 16, 2018

I come from a Java background, and it's sometimes really confusing, because TypeScript's type system can do really weird things. It's really mighty. Sometimes I really miss some of it's features when writing Java code. And sometimes my brain starts to hurt when I look at some really complex TypeScript typings, like "extends with generics" or "infer" :D

But the good thing is: As long it's part of some external library (like yours) I don't have to care about the really complex stuff. Thanks for that!

@aleclarson
Copy link
Member

🎉 This issue has been resolved in version 1.9.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aleclarson
Copy link
Member

Whoops, I forgot to remove the void from the other two function signatures. Let me know if it's a problem.

@benneq
Copy link
Author

benneq commented Dec 16, 2018

So far 1.9.2 works! But I guess, I have another issue with typings. I'll open a new issue in a few minutes.

Maybe you can fix the void stuff then for 1.9.3 :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants