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

[Types] Strictly typed store allows invalid values #95

Closed
xzilja opened this issue Mar 14, 2020 · 12 comments · Fixed by #174
Closed

[Types] Strictly typed store allows invalid values #95

xzilja opened this issue Mar 14, 2020 · 12 comments · Fixed by #174

Comments

@xzilja
Copy link

xzilja commented Mar 14, 2020

I might be missing something, but shouldn't following disallow invalidA and invalidB values to be used in a store? As it is at the moment I don't get any error about them.

import create from 'zustand';

interface Store {
  count: number;
  increase: () => void;
}

const [useStore] = create<Store>(set => ({
  count: 0,
  invalidA: 0,
  increase: () => set(state => ({ count: state.count + 1 })),
  invalidB: () => set({ count: 0 })
}));

export default useStore;
@JeremyRH
Copy link
Contributor

JeremyRH commented Mar 15, 2020

I don’t understand why TypeScript does this. You can experiment with the behavior here.

Here's a workaround:

const [useStore] = create<Store>((set): Store => ({ // additional type declaration
  count: 0,
  invalidA: 0, // errors
  increase: () => set(state => ({ count: state.count + 1 }))
}));

@clawoflight
Copy link
Contributor

I assume that this is due to the structural/duck typing - any superset of Store is assignable to Store because it has all properties of Store?

@JeremyRH
Copy link
Contributor

You're right. I learned it has to do with structural typing. Excess properties are allowed with structural typing. TypeScript does have excess property checking but it's only when explicitly defining an object of a specific type. Here is a good article about it:
https://medium.com/@lemoine.benoit/why-does-typescript-sometimes-fails-to-type-check-extra-properties-fd230ebbc295

@dai-shi
Copy link
Member

dai-shi commented Aug 15, 2020

I think this is resolved.

@dai-shi dai-shi closed this as completed Aug 15, 2020
@csr632
Copy link

csr632 commented Aug 25, 2020

I got some typescript issue. Zustand can't infer the type of State from my creator. Example: https://codesandbox.io/s/test-zustand-74jze?file=/src/App.tsx

export function createItem(name: string) {
  const api = create((set) => {
    return {
      name,
      count: 1,
      setName: (newName: string) => {
        // set((prev) => {
        //   // prev
        // });
      }
    };
  });

  console.log(api.getState().invalid); // expect this line to trigger ts error, but it doesn't
}

@dai-shi
Copy link
Member

dai-shi commented Aug 25, 2020

@csr632 Yeah, I noticed that too and it's annoying for me personally.
I guess it's because of recursive type definition and TS can't infer types. We need TS experts help on it.

If you remove set, the store type will be inferred as you expect.

export function createItem(name: string) {
  const api = create(() => {
    return {
      name,
      count: 1,
      setName: (newName: string) => {
        // set((prev) => {
        //   // prev
        // });
      }
    };
  });

Only solution I have now is to manually type state: https://codesandbox.io/s/test-zustand-forked-mo5rs

@csr632
Copy link

csr632 commented Sep 2, 2020

Maybe related to this ts design limitation: microsoft/TypeScript#25092 (comment)

Could we re-design zustand's API so that it doesn't encourage user to write "context-sensitive function"?

const api = create((set) => { // this is a context-sensitive because it has untyped function parameters ”set“
    return {
      name,
      count: 1,
    };
  });

@dai-shi
Copy link
Member

dai-shi commented Sep 2, 2020

I'm not sure if I understand the term context-sensitive function.
Could we do anything without a breaking change?

@csr632
Copy link

csr632 commented Sep 2, 2020

I'm not sure if I understand the term context-sensitive function.

As microsoft/TypeScript#25092 (comment) said:

  • a context-sensitive function is a function with untyped function parameters
  • ts can't infer type from a context-sensitive function

In this case:

const api = create((set) => { // this is a context-sensitive because it has untyped function parameters ”set“
    return {
      name,
      count: 1,
    };
  });

So create can't infer any type infomation from the function.

Could we do anything without a breaking change?

Yes, we should never use the params:

const api = create(() => { // this is NOT a context-sensitive function because it doesn't list any params
   // we should never use the params
    return {
      name,
      count: 1,
      setName: (newName: string) => {
        // we use `api.set` instead of the `set` from param
        api.setState((prev) => ({
          count: prev.count + 1
        }));
      }
    };
  });

Checkout codesandbox.

But I think this is somehow ugly...

@dai-shi
Copy link
Member

dai-shi commented Sep 2, 2020

Thanks. I got it. That's what I noted recursive type definition. Now I learned the proper term: context sensitive.

Thanks for your example.
So, this is possible: https://codesandbox.io/s/test-zustand-forked-9b789

import create from "zustand";

const useStore = create(() => {
  const set = (...args: Parameters<typeof useStore.setState>) =>
    useStore.setState(...args);
  return {
    name: "",
    count: 1,
    setName: (newName: string) => {
      set((prev) => ({
        name: newName
      }));
    }
  };
});

This pattern doesn't allow us to add middleware. So, I'm not sure if we recommend it as a first class usage.

@dai-shi dai-shi reopened this Sep 2, 2020
@dai-shi
Copy link
Member

dai-shi commented Sep 2, 2020

  1. structural typing
  2. context sensitive function

They are basically technical limitations, but could we somehow mitigate them?

@dai-shi dai-shi mentioned this issue Sep 3, 2020
@dai-shi
Copy link
Member

dai-shi commented Sep 3, 2020

@csr632 Check #174 . It would mitigate both issues with new middleware.

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 a pull request may close this issue.

5 participants