fix(types)!: require complete state if setState's replace flag is set

[Yonom]

Related Issues or Discussions

Fixes #2578

Summary

Update the setState type to conditionally require a non-partial T if the replace argument of the function is set to true. This change also updates the types for set inside create((set) => ({ ... })) callbacks.

Previously, store.setState({}, true) was considered valid, even though it would result in an invalid state. This fixes that issue.

My suggestion is to consider this a breaking change to be on the safe side.

Check List

• pnpm run prettier for formatting code and docs

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
zustand-demo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 29, 2024 0:21am

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[Yonom]

The user is now presented with two function signatures. These seem to be sorted by the length of their definition text, which causes the default to suggest replace: true, since it has a shorter definition

[charkour]

I believe this is expected behavior. See "overwriting state" on the Readme.

Thank you

[charkour]

However, I do advocate for this change.

[dai-shi]

which causes the default to suggest replace: true, since it has a shorter definition

Oh, that sounds pretty bad. replace would only be used less than say 1% users...

[charkour]

I disagree with defaulting to replace to true.

[dai-shi]

I disagree with defaulting to replace to true.

It's not about changing the logic. It's just about typing. Yet...

[dai-shi]

https://tsplay.dev/WJ0MlN

Typing f(:

Typing f({:

Hmm, maybe it's fine.

[Yonom]

Three suggestions to fix the order of overloads:

Variant 1

-    replace: true,
+    replace: true | undefined,

Variant 1

Downside: true | undefined might confuse users.

Variant 2

-    replace?: false | undefined,
+    replace?: false,

Variant 2

Downside: when tsconfig compilerOption exactOptionalPropertyTypes is set, undefined can no longer be passed to replace. (small breaking change, not sure if it has any real world impact, can be worked around by users with replaceOrUndefined ?? false)

Variant 3

-    replace?: false | undefined,
+    replace?: false,
...
+  _(
+    partial: T | Partial<T> | { _(state: T): T | Partial<T> }['_'],
+    replace?: undefined,
+  ): void

Variant 3

Downside: Somewhat unnecessary third overload.

[dai-shi]

My preference: Original or Variant 3.

[Yonom]

@dai-shi

Typing f({:

In my Visual Studio Code (MacOS, VSC v1.89.1), IntelliSense does not switch to the second overload upon typing "{":

(but I see the behavior you describe in the in-browser ts playground)

[Yonom]

Variant 2 - Downside: when tsconfig compilerOption exactOptionalPropertyTypes is set, undefined can no longer be passed to replace. (small breaking change, not sure if it has any real world impact, can be worked around by users with replaceOrUndefined ?? false)

Actually: this does not seem to affect function arguments! replace?: false turns into replace?: false | undefined despite exactOptionalPropertyTypes being enabled. My bad!

Variant 2 + exactOptionalPropertyTypes enabled

As far as I can tell from my testing, removing the explicit | undefined from the definition fixes the ordering issue without causing any behavior change (tested with https://www.npmjs.com/package/@tsconfig/strictest). Therefore, Variant 2 is my preference

[dai-shi]

LGTM

[dai-shi]

I see. I too thought | undefined is required somehow, but this is functions argument, not object property.

[dai-shi]

See CI errors 😄

[Yonom]

This fix breaks the following code:

const wrapper: StoreApi<T>["setState"] = (partial, replace) => {
  return store.setState(partial, replace);
}

I can imagine this kind of code existing in other libraries or userland...

Here's a playground

Any ideas?

[dai-shi]

Yeah, function overloading is always tricky. No, I don't have any ideas.

[dai-shi]

@Yonom I think we can merge this for v5, and based on feedback, we might relax the type in the future.
Can you address CI issues please?

[Yonom]

The old typescript tests were failing because old typescript versions <=5.2 resolve the following to never if the function does not have at least two overloads.

{ 
  (...a: infer A1): infer Sr1;
  (...a: infer A2): infer Sr2;
}

I updated the types of devtools and immer middleware to also specify two overloads for their wrapped setState function, solving this issue (and improving the types for these middleware at the same time)

---

There is another solution to this problem, which is to first infer the parameters and return types as a tuple array, then filter the unknown types and finally turn that into a union. microsoft/TypeScript#32164 (comment)
For now, I opted to keep things simple and not implement that workaround.

[Yonom]

After the last commit (update setState types for devtools and immer), the following test is failing:

it('StateCreator<T, [StoreMutatorIdentfier, unknown][]> is StateCreator<T, []>', () => {
  interface State {
    count: number
    increment: () => void
  }

  const foo: <M extends [StoreMutatorIdentifier, unknown][]>() => StateCreator<
    State,
    M
  > = () => (set, get) => ({
    count: 0,
    increment: () => {
      set({ count: get().count + 1 }) // <-- type error here
    },
  })

  create<State>()(persist(foo(), { name: 'prefix' }))
})

This expression is not callable.
  Each member of the union type 
  '{ 
    (partial: State | Partial<State> | ((state: State) => State | Partial<State>), replace?: false | undefined): void; 
    (state: State | ((state: State) => State), replace: true): void; 
  } 
  | 
  { 
    <A extends string | { type: string; }>(
      partial: State | Partial<State> | ((state: State) => | Partial<State>), 
      replace?: false | undefined, 
      action?: A | undefined
    ): void; 
    <A extends string | { type: string; }>(
      state: State | ((state: State) => State), 
      replace: true, 
      action?: A | undefined
    ): void; 
  } 
  | 
  { 
    (nextStateOrUpdater: State | Partial<State> | ((state: WritableDraft<State>) => void), shouldReplace?: false | undefined): void; 
    (nextStateOrUpdater: State | ((state: WritableDraft<State>) => void), shouldReplace: true): void; 
  }' 
  has signatures, but none of those signatures are compatible with each other.

It seems like typescript is having issues consolidating multiple generic functions setState<A extends string | { type: string; }>, the cleanest fix I could find was to remove the generic parameter on setState and replace it with:

type Action =
  | string
  | {
      type: string
      [x: string | number | symbol]: unknown // <-- allows for arbitrary values
    }

This is another minor breaking change, I would love to avoid this incase you have a better idea for a workaround

[Yonom]

All tests are passing again and I tested also with old typescript 4.5.5.

Ready for review @dai-shi

[devanshj]

FWIW there's already a PR with the same goal by me here.

This PR's approach would break things like this...

let foo = Math.random() > 0.5 ? true : false
store.setState({ bears: 5 }, foo) // error

Here's a minimal reproduction...

declare const f: {
  (a: true): void
  (a: false): void
}

let foo = Math.random() > 0.5 ? true : false
f(foo) // error

[Yonom]

@devanshj Thanks for pointing this out.

The following cases exist:

1. state: T | Partial<T>, replace?: false
2. state: T, replace: true <- goal of this PR
3. state: T | Partial<T>, replace: boolean <- breaks
4. state: T, replace: boolean <- breaks

This PR breaks type support for cases 3 and 4.

I assume the distribution of real world uses of each of these cases to be:

1. 95 %
2. 5 %
3. 0.1 %
4. 0 %

Case 3 is unfortunate. In my opinion, breaking case 3 to catch errors in case 2 is well worth it. (Case 3 is easily worked around)
I literally cannot see a use case for case 4, I therefore would not fix it.

---

Workaround for case 3:

- store.setState(partialOrFull, replace);
+ store.setState(partialOrFull, replace as any);

---

We could add support for case 4:

type SetStateInternal<T> = {
  _(
    partial: T | Partial<T> | { _(state: T): T | Partial<T> }['_'],
    replace?: false,
  ): void
  _(
    state: T | { _(state: T): T }['_'],
-   replace: true,
+   replace: boolean,
  ): void
}['_']

TS Play

Pro: fixes case 4
Con: it displays replace: boolean instead of replace: true for the second overload

I don't see the point in case 4 and therefore suggest keeping replace: true.

[dai-shi]

FWIW there's already a PR with the same goal by me here.

Oh, yeah...

Nothing is ideal.

(Case 3 is easily worked around)

How is it? I think it should be documented somewhere in ./docs as well as the migration guide.
If that is done, I think we can merge this PR as v5 is a good opportunity for such breaking change.

[dai-shi]

@Yonom ☝️ Sorry, if I wasn't clear. Can you add a note in docs and v5 migration guide please?

[dai-shi]

@Yonom A friendly reminder.

[Yonom]

Sorry about the delay. Done @dai-shi

[dai-shi]

Thanks.

It's my bad #2138 (comment), but the migration doc is under docs/guides currently. (I will move it around after merging the v5 branch.)

Can you move your notes into it?

[Yonom]

I also missed that! updated now

[dai-shi]

Looks great to me! Thanks for your contribution!

[Yonom]

Whew! Thanks a lot for all the guidance along the way, this was way more complicated than originally expected 😀