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

Suggestion from my own confusion #1046

Closed
elilondon opened this issue Jun 30, 2022 · 13 comments · Fixed by #1090
Closed

Suggestion from my own confusion #1046

elilondon opened this issue Jun 30, 2022 · 13 comments · Fixed by #1090

Comments

@elilondon
Copy link

The docs say to use this StateCreator<MySlice, [["zustand/devtools", never]], []> for independent slices when using middleware but the docs don't show how to use it in an example. Couple that with trying to partialize certain things and I believe there should be clarity there.

If I have two slices (independent) and want to persist certain things from each one of the two slices, it may be helpful to show the intended way to accomplish this.

Thanks a lot for your time!

@dai-shi
Copy link
Member

dai-shi commented Jun 30, 2022

@devanshj can take it please?

@devanshj
Copy link
Contributor

devanshj commented Jul 3, 2022

I believe there should be clarity there.

Fair enough, you caught me being lazy :P I think #1050 would suffice, let me know if something is unclear.

@elilondon
Copy link
Author

I believe there should be clarity there.

Fair enough, you caught me being lazy :P I think #1050 would suffice, let me know if something is unclear.

Apologies, I meant more of a working example rather than an explanation as I am truly unsure of the format that is intended. I will expand on what I am trying to do when I get a chance tomorrow. I am sure I am missing something simple.

@devanshj
Copy link
Contributor

devanshj commented Jul 5, 2022

I just tried writing myself and there's an error that is technically on our end. So now I follow why you were facing problems, not your fault though. The code would look something like this...

import create, { StateCreator } from "zustand"
import { persist } from "zustand/middleware"

// ----------
// Bear

interface BearSlice {
  bears: number
  persistMe: number
  addBear: () => void
}

const createBearSlice: StateCreator<BearSlice, [["zustand/persist", PersistedBearSlice]], []> = (set) => ({
  bears: 0,
  addBear: () => set((state) => ({ bears: state.bears + 1 })),
  persistMe: 0
})

interface PersistedBearSlice
  extends Pick<BearSlice, "persistMe"> {}
// or...
// type PersistedBearSlice = Pick<BearSlice, "persistMe">

const partializeBearSlice = (s: BearSlice): PersistedBearSlice =>
  ({ persistMe: s.persistMe })


// ----------
// Fish

interface FishSlice {
  fishes: number
  persistMeToo: string
  addFish: () => void
}

const createFishSlice: StateCreator<FishSlice, [["zustand/persist", PersistedFishSlice]], []> = (set) => ({
  fishes: 0,
  addFish: () => set((state) => ({ fishes: state.fishes + 1 })),
  persistMeToo: ""
})

interface PersistedFishSlice
  extends Pick<FishSlice, "persistMeToo"> {}

const partializeFishSlice = (s: FishSlice): PersistedFishSlice =>
  ({ persistMeToo: s.persistMeToo })


// ----------
// Bear & Fish

const useStore = create<BearSlice & FishSlice>()(persist((...a) => ({
  ...createBearSlice(
    // @ts-ignore
    ...a
  ),
  ...createFishSlice(
    // @ts-ignore
    ...a
  ),
}), {
  name: "test",
  partialize: s => ({ ...partializeBearSlice(s), ...partializeFishSlice(s) })
}))

I hope this is what you were looking for. Those two // @ts-ignore can't be avoided right now, I'll see if the types can be improved so that they are not needed.

Here's the minimal version of the same error:

import create, { StateCreator } from "zustand"
import { persist } from "zustand/middleware"

type Foo = { foo: number }
type FooMore = Foo & { more: number }
declare const createFoo: StateCreator<Foo, [["zustand/persist", unknown]], []>

const useFooMore = create<FooMore>()(persist((...a) => ({
  ...createFoo(...a),
  more: 0
})))

The fix likely is making some methods in the PersistOptions type bivariant. And to be clear TypeScript is pointing out correct unsoundness, writing code like this can cause exceptions, but they are edge cases and we want to be pragmatic and allow compiling this.

@elilondon
Copy link
Author

@devanshj Ok I really appreciate this. Those were in fact the issues I was facing and the typing errors I was having. Thanks a lot!

@devanshj
Copy link
Contributor

devanshj commented Jul 5, 2022

You're welcome! And thanks to you too for reporting.

@devanshj
Copy link
Contributor

devanshj commented Jul 15, 2022

Okay so first let me explain what's the problem here then we can see in what ways we can fix this.

TypeScript here is right for not compiling the code because it detects that it's unsound (ie can cause runtime exceptions)...

import create, { StateCreator } from "zustand";
import { persist } from "zustand/middleware";

type Foo = { foo: number; causeTrouble: () => void };
type FooMore = Foo & { more: number };
const createFoo: StateCreator<Foo, [["zustand/persist", unknown]], []> = (
  get,
  set,
  store
) => {
  return {
    foo: 0,
    causeTrouble: () => {
      store.persist.setOptions({
        migrate: () => ({ foo: 0, causeTrouble: () => {} }),
        merge: (a, b) => a as typeof b
      });
    }
  };
};

localStorage.removeItem("foo");

const useFooMore = create<FooMore>()(
  persist(
    (...a) => ({
      ...createFoo(...a),
//                 ~~~~ [1]
      more: 0
    }),
    { name: "foo", version: 1 }
  )
);

useFooMore.getState().causeTrouble();
localStorage.setItem("foo", `{ "version": 2, "state": {} }`);
useFooMore.persist.rehydrate();
useFooMore.getState().more.toFixed();
// Runtime TypeError: Cannot read properties of undefined (reading 'toFixed')

/* [1]:
Argument of type 'Write<StoreApi<FooMore>, StorePersist<FooMore, unknown>>' is not assignable to parameter of type 'Write<StoreApi<Foo>, StorePersist<Foo, unknown>>'.
  Type 'Write<StoreApi<FooMore>, StorePersist<FooMore, unknown>>' is not assignable to type 'StorePersist<Foo, unknown>'.
    The types of 'persist.setOptions' are incompatible between these types.
      Type '(options: Partial<PersistOptions<FooMore, unknown>>) => void' is not assignable to type '(options: Partial<PersistOptions<Foo, unknown>>) => void'.
        Types of parameters 'options' and 'options' are incompatible.
          Type 'Partial<PersistOptions<Foo, unknown>>' is not assignable to type 'Partial<PersistOptions<FooMore, unknown>>'.
            Types of property 'migrate' are incompatible.
              Type '((persistedState: unknown, version: number) => Foo | Promise<Foo>) | undefined' is not assignable to type '((persistedState: unknown, version: number) => FooMore | Promise<FooMore>) | undefined'.ts(2345)
*/

You can run and see the exception in this codesandbox. The culprit here is that migrate should return the whole state ie FooMore but for createFoo the whole state is Foo, hence we get the compile error at createFoo(...a). But we can make this error better and shift it to the correct place by using the "correct" createFoo type that doesn't lie about the whole state ie make this change...

- StateCreator<Foo, [["zustand/persist", unknown]], []>
+ StateCreator<FooMore, [["zustand/persist", unknown]], [], Foo>
import create, { StateCreator } from "zustand";
import { persist } from "zustand/middleware";

type Foo = { foo: number; causeTrouble: () => void };
type FooMore = Foo & { more: number };
const createFoo: StateCreator<FooMore, [["zustand/persist", unknown]], [], Foo> = (
  get,
  set,
  store
) => {
  return {
    foo: 0,
    causeTrouble: () => {
      store.persist.setOptions({
        migrate: () => ({ foo: 0, causeTrouble: () => {} }),
//      ~~~~~~~ [1]
        merge: (a, b) => a as typeof b
      });
    }
  };
};

localStorage.removeItem("foo");

const useFooMore = create<FooMore>()(
  persist(
    (...a) => ({
      ...createFoo(...a),
      more: 0
    }),
    { name: "foo", version: 1 }
  )
);

useFooMore.getState().causeTrouble();
localStorage.setItem("foo", `{ "version": 2, "state": {} }`);
useFooMore.persist.rehydrate();
useFooMore.getState().more.toFixed();
// Runtime TypeError: Cannot read properties of undefined (reading 'toFixed')

/* [1]:
Type '() => { foo: number; causeTrouble: () => void; }' is not assignable to type '(persistedState: unknown, version: number) => FooMore | Promise<FooMore>'.
  Type '{ foo: number; causeTrouble: () => void; }' is not assignable to type 'FooMore | Promise<FooMore>'.
    Type '{ foo: number; causeTrouble: () => void; }' is not assignable to type 'FooMore'.
      Property 'more' is missing in type '{ foo: number; causeTrouble: () => void; }' but required in type '{ more: number; }'.
*/

Now the error is in the "right" position ie at migrate and there's no error in createFoo(...a).

So now one way of solving this issue ie independent slices pattern not compiling is to not use independent slices pattern and use interdependent slices pattern. Of course the slices can be indepedent in the runtime code, but their types are true and we don't have any problem and the following code compiles...

import create, { StateCreator } from "zustand"
import { persist } from "zustand/middleware"

type Foo = { foo: number }
type FooMore = Foo & { more: number }
declare const createFoo: StateCreator<FooMore, [["zustand/persist", unknown]], [], Foo>

const useFooMore = create<FooMore>()(persist((...a) => ({
  ...createFoo(...a),
  more: 0
})))

Now the downside of this is that all slices will need to import the state type of all other slices. So let's say you have slices A, B and C cross three files you'll have to import B's and C's state types in A and so on. Now one may say it's bad that A has to import B and C when they are not even related, A should not even know or care that B and C exist... Now this is sounds correct but the math, the type error and the runtime error above proves that it's incorrect and A must know B's and C's existence because it can do stuff like setOptions({ migrate: () => ... }).

Now one can say "Okay then remove the feature of setOptions({ migrate: () => ... }) and give me truly independent slices" and yes this is the second option.

What we do create a type SliceCreator that would remove all operations (setOptions({ migrate: () => ... })) on store that covariantly depend on the state type, in simpler words remove all operations that require you to provide the state type. You see migrate requires you to provide the state type, the slice thinks my slice is the whole state and it compiles, but actually the whole state is bigger, so we need to disallow slices to provide the state at all to make them truly independent. And ofc this new SliceCreator will make the internal types a bit more complicated. And tbc SliceCreator would only disallow changing migrate (and some other covariant) options, not all options.

So to recap we have two options:

1. No more independent slices, only interdependent slices.

Pro: Internal types remain as it is

Con: Users can't create truly independent slices because they have to be depend upon other slices. Although the dependence can be less cumbersome by importing the whole store type instead of all slices, that is to say instead of this...

// @file state/a.ts
import { B } from "./b"
import { C } from "./c"
export type A = {}

// @file state/b.ts
// ...

// @file state/c.ts
// ...

... we can do this ...

// @file state/index.ts
import { A } from "./a"
import { B } from "./b"
import { C } from "./c"
export type State = A & B & C

// @file slice/a.ts
import { State } from "."
export type A = {}

// @file slice/b.ts
// ...

// @file slice/c.ts
// ...

And now the A doesn't have to import B, C, D, E, etc. it has to import only the final state type, only the file that has final state type has to import all slices.

2. Create a new type (say SliceCreator) to have truly independent slices.

Pro: We now have truly independent slices that can be totally unaware of the other slices of the store.

Con: The internal types become more complicated.

I personally would suggest to go with option 1, because in some sense it's closer to the truth, in reality slices aren't exactly "independent"... Not only as we saw here with migrate but in many other ways like if A slice has property foo then B slice should not have property foo or else it'll override A's property, setState(..., true) replacing the whole store not the whole slice, the parameter received belonging to the store with other slices not just the current one, etc. We can do type work to express truly independent slices, but it'll be rather complex (ignore my commit that you can see above, it'll be different and more complex than that).

And ofc there's always the YOLO option, that is to tell our users to preferably write interdependent slices but if they want to write independent slices then they can do so by write // @ts-ignore above createSlice(...a) (and then being careful without the help of compiler to not use setOptions({ migrate: () => ... }) and other covariant operations).

Also I hope I've explained the issue correctly, let me know if you want me to clarify something. And tbc I'm mainly addressing this to @dai-shi here haha.

@dai-shi
Copy link
Member

dai-shi commented Jul 16, 2022

Thanks so much for the clarification.

  1. No more independent slices, only interdependent slices.

Go with this. It's mostly by design. This is the same mental model that we gave up namespacing. #178.
That reminds me, @devanshj if you can help typing zustand-lens by @dhmk083, it would be much appreciated. I'm not even sure how easy it is.

@devanshj
Copy link
Contributor

devanshj commented Jul 16, 2022

Cool, in that case we just have to remove independent slices pattern from the docs, opened #1090. And sure I'll check out zustand-lens and see what I can do soon.

@dhmk083
Copy link

dhmk083 commented Jul 18, 2022

Main issue with zustand-lens is that you can have for example a lens which is supposed to work with immer middleware and you can pass it to a store without immer middleware and you won't get a compile-time error. It would be nice to have an error if types are mismatched.

And I'm starting to think that if one wants truly independent slices, then multiple store pattern might be a good idea. Persist middleware can have unique names, so it won't collide with each other. The only issue may be with devtools, but I think it can be adapted to support multiple stores.

Btw, regarding to this issue I can suggest a different pattern to persist slices, which I use. Each slice can define a symbol with persisting options and then a special function collects these metadata into a final options object.

Here is an example for partialize option:

const PARTIALIZE = Symbol()

const bearsSlice = (set, get) => ({
  bears: 0,
  addBear() {},
  skipThis: true,
  [PARTIALIZE]: ({skipThis, ...keep}) => keep
})

const store = create(persist(withLenses({
  bears: lens(bearsSlice)
}), {
  name: 'state',
  partialize: s => walkObjects(s, v => v[PARTIALIZE]?.(v) ?? v),
  merge: (saved, state) => mergeDeep(state, saved) // this is only needed to prevent overwriting nested functions while rehydrating
}))

function walkObjects(x, fn, path = [] as any[]) {
  if (isPlainObject(x)) {
    const res = fn(x, path);

    if (isPlainObject(res)) {
      for (const k in res) {
        res[k] = walkObjects(res[k], fn, path.concat(k));
      }
    }

    return res;
  } else return x;
}

@chenningg
Copy link

chenningg commented Oct 5, 2022

Hi, I'm trying to use persist middleware with the interdependent slices pattern as such:

export type RootStore = AppSettingsSlice

// Create singular global store with slices and middleware.
const useStore = create<RootStore>()(persist((...a) => ({
  ...createAppSettingsSlice(
    ...a
  ),
})))

and in the slice:

// Persist application settings.
export interface AppSettingsSlice {
  bears: number
  addBear: () => void
}

export const createAppSettingsSlice: StateCreator<
  RootStore,
  [["zustand/persist", AppSettingsSlice]],
  [],
  AppSettingsSlice
> = (set) => ({
  bears: 0,
  addBear: () => set((state) => ({ bears: state.bears + 1 })),
})

However, I'm still getting the typescript error on the persist(...a) in the root:

Argument of type 'Write<StoreApi<AppSettingsSlice>, StorePersist<AppSettingsSlice, unknown>>' is not assignable to parameter of type 'Write<StoreApi<AppSettingsSlice>, StorePersist<AppSettingsSlice, AppSettingsSlice>>'.
  Type 'Write<StoreApi<AppSettingsSlice>, StorePersist<AppSettingsSlice, unknown>>' is not assignable to type 'StorePersist<AppSettingsSlice, AppSettingsSlice>'.
    The types of 'persist.getOptions().deserialize' are incompatible between these types.
      Type '((str: string) => StorageValue<unknown> | Promise<StorageValue<unknown>>) | undefined' is not assignable to type '((str: string) => StorageValue<AppSettingsSlice> | Promise<StorageValue<AppSettingsSlice>>) | undefined'.ts(2345)

How do I fix this without having to do // @ts-ignore? Or do I still have to do [["zustand/persist", unknown]]?

@devanshj
Copy link
Contributor

devanshj commented Oct 5, 2022

Or do I still have to do [["zustand/persist", unknown]]?

Yes you'll have to write ["zustand/persist", unknown] instead of ["zustand/persist", AppSettingsSlice]. It's a known issue that can't be really solved, it's not a big deal because it doesn't compromise the types for you unless you're using store.persist.setOptions iirc.

@chrism1996
Copy link

I just tried writing myself and there's an error that is technically on our end. So now I follow why you were facing problems, not your fault though. The code would look something like this...

import create, { StateCreator } from "zustand"
import { persist } from "zustand/middleware"

// ----------
// Bear

interface BearSlice {
  bears: number
  persistMe: number
  addBear: () => void
}

const createBearSlice: StateCreator<BearSlice, [["zustand/persist", PersistedBearSlice]], []> = (set) => ({
  bears: 0,
  addBear: () => set((state) => ({ bears: state.bears + 1 })),
  persistMe: 0
})

interface PersistedBearSlice
  extends Pick<BearSlice, "persistMe"> {}
// or...
// type PersistedBearSlice = Pick<BearSlice, "persistMe">

const partializeBearSlice = (s: BearSlice): PersistedBearSlice =>
  ({ persistMe: s.persistMe })


// ----------
// Fish

interface FishSlice {
  fishes: number
  persistMeToo: string
  addFish: () => void
}

const createFishSlice: StateCreator<FishSlice, [["zustand/persist", PersistedFishSlice]], []> = (set) => ({
  fishes: 0,
  addFish: () => set((state) => ({ fishes: state.fishes + 1 })),
  persistMeToo: ""
})

interface PersistedFishSlice
  extends Pick<FishSlice, "persistMeToo"> {}

const partializeFishSlice = (s: FishSlice): PersistedFishSlice =>
  ({ persistMeToo: s.persistMeToo })


// ----------
// Bear & Fish

const useStore = create<BearSlice & FishSlice>()(persist((...a) => ({
  ...createBearSlice(
    // @ts-ignore
    ...a
  ),
  ...createFishSlice(
    // @ts-ignore
    ...a
  ),
}), {
  name: "test",
  partialize: s => ({ ...partializeBearSlice(s), ...partializeFishSlice(s) })
}))

I hope this is what you were looking for. Those two // @ts-ignore can't be avoided right now, I'll see if the types can be improved so that they are not needed.

Here's the minimal version of the same error:

import create, { StateCreator } from "zustand"
import { persist } from "zustand/middleware"

type Foo = { foo: number }
type FooMore = Foo & { more: number }
declare const createFoo: StateCreator<Foo, [["zustand/persist", unknown]], []>

const useFooMore = create<FooMore>()(persist((...a) => ({
  ...createFoo(...a),
  more: 0
})))

The fix likely is making some methods in the PersistOptions type bivariant. And to be clear TypeScript is pointing out correct unsoundness, writing code like this can cause exceptions, but they are edge cases and we want to be pragmatic and allow compiling this.

is there any workaround for avoiding these @ts-ignore? I got the same warning I'm not sure if it's fixed yet? I dont like to get these warnings in TS, even when I'm implement this on a new project from my work and all my team mates will ask why we get them, thanks

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.

6 participants