-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
breaking(types): Add higher kinded mutator types #725
Conversation
Okay so let me explain the recent commits a bit...
This one is fairly simple, we're making less assumptions about the the "parent" The type SkipTwoBasic<T> =
T extends [] ? [] :
T extends [unknown] ? [] :
T extends [unknown, unknown, ...infer A] ? A :
never ... won't work because the way subtyping works ie
I don't know if you remember but this PR actually bring a new feature that whenever users are replacing the state, they'll have to pass the whole state, so now we have more type safety. I also added a test for this: 061a057. But the problem is this feature only work if there are no higher kinded mutators that change declare module '../vanilla' {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
interface StoreMutators<S, A> {
'zustand/devtools': WithDevtools<S>
}
}
type WithDevtools<S> =
Write<Cast<S, object>, StoreSetStateWithAction<S>>
type StoreSetStateWithAction<S> =
S extends { setState: (...a: infer A) => infer R }
? { setState: (...a: [...a: A, actionType?: string | { type: unknown }]) => R }
: never
type Write<T extends object, U extends object> = Omit<T, keyof U> & U
type Cast<T, U> = T extends U ? T : U Here the problem is that when you use Unfortunately this is a TypeScript limitation, and there's no workaround to this. Hence what we do now is add special cases for the types of type StoreSetStateWithAction<S> =
S extends { getState: () => infer T, setState: (...a: infer A) => infer R }
? A extends [Partial<T> | ((state: T) => Partial<T>), (boolean | undefined)?]
? /* redefining vanilla setState but now with `actionType` */ :
A extends [Partial<T> | ((state: Draft<T>) => void), (boolean | undefined)?]
? /* redefining immer setState but now with `actionType` */ :
{ setState: (...a: [...a: A, actionType?: string | { type: unknown }]) => R }
// fallback, this would lose replace-aware feature
: never And now the replace feature would work even for devtools (with vanilla or immer). You can see I've added those tests for it in Additionally, Not only that if you notice we don't return the result from "parent" ;(api.setState as NamedSet<S>) = (state, replace, nameOrAction) => {
- set(state, replace)
+ let r = set(state, replace)
- if (!isRecording) return
+ if (!isRecording) return r
extension.send(
nameOrAction === undefined
? { type: devtoolsOptions.anonymousActionType || 'anonymous' }
: typeof nameOrAction === 'string'
? { type: nameOrAction }
: nameOrAction,
get()
)
+ return r
} But currently give we don't I also replaced the Although I think I'll make this tiny runtime change tomorrow (will revert if you don't like it). Just to make the middleware more open and extensible. And immer middleware (because I authored it :P) already returns the result of To elaborate on "more open and more extensible" I mean it's possible that users may have some middleware which has a special I understand I'm being a perfectionist here, but that's who I am :P (when it comes to code)
Ha we caught some bugs in the tests because of this additionally type safety. Previously as I said devtools wouldn't have the replace feature but now it does, so we caught passing partial state when we were replacing.
Okay now this one is a bit lengthy. And it's time to sleep so I'll explain tomorrow xD I know the types have gotten a bit more complex but I think it's worth it, we've already come this far so a little more correctness is doesn't harm. |
well, I know, but keep in mind that if you make things too complicated and others can't follow, we will eventually remove that code in the future abandoning your work and effort. Non-understandable code is worse than non-perfect code. So, you should find the balance between perfectness and understandability. tbh, you already went too far.
SkipTwo is complex, but understandable because the goal is clear.
I actually like it. >
Can we give this up, and make things less complicated? Using |
I understand what you're getting at, but I don't think correctness of code and understandability of code are always mutually exclusive. It depends, here the assumption is the maintainers have medium-to-high typescript knowledge (and have my detailed documentations), moreover the type-level code requires zero maintenance (apart from future bugs which are unlikely and I'll mostly be available), so we can have both. Of course if these conditions are not met then it's perfectly reasonable to delete my code, I don't mind that nor do I think my work goes in vain.
Keeping the limitation would not reduce the complexity by any significant margin, it'll only remove the two added branches (ie vanilla and immer) in The alternative is that users will have to be careful themselves and pass the whole state when replacing, the compiler won't help them. I'd suggest not to compromise, but you can take the final call. |
Okay let me give a little introduction on variance first (I'll not go too deep though). Imagine if you have type GetState<T> = () => T
// "b" is a subtype of "a" | "b"
let t0: "a" | "b" = {} as "b"
// therefore GetState<"b"> is a subtype of GetState<"a" | "b">
let t1: GetState<"a" | "b"> = {} as GetState<"b"> And if you have type SetState<T> = (t: T) => void
// "b" is a subtype of "a" | "b"
let t0: "a" | "b" = {} as "b"
// therefore SetState<"b"> is a supertype of SetState<"a" | "b">
// or in otherwords SetState<"a" | "b"> is a subtype of SetState<"b">
let t1: SetState<"b"> = {} as SetState<"a" | "b"> Now what if you combine these both, the resulting type would become invariant (means neither covariant neither contravariant) which destroys the subtype relation... interface Store<T> {
getState: () => T
setState: (t: T) => void
}
let t0: Store<"a"> = {} as Store<"a" | "b"> // does not compile
let t1: Store<"a" | "b"> = {} as Store<"a"> // also does not compile Now although this is correct and sound but it's not pragmatic. What this means is no two stores can be subtype of each other. And hence you can't write any abstractions over interface Store<T> {
getState: () => T
setState: (t: T) => void
}
declare const useStore:
<S extends Store<unknown>>(t: S) =>
S extends { getState: () => infer T } ? T : never
declare const myStore: Store<{ count: number }>
useStore(myStore)
// does not compile
// because Store<{ count: number }> is not a subtype Store<unknown>
// { count: number } is a subtype of unknown, so had Store<T> been covariant
// Store<{ count: number }> would also be a subtype Store<unknown> Now how to fix this? We actually want Now we get rid of type SetStateContravariant<T> = (t: T) => void
type SetStateBivariant<T> = { _(t: T): void }["_"]
// type Foo = { x: (t: T) => void } // here x is property
// type Bar = { x(t: T): void } // here x is method
// both are different, methods have bivariant arguments
let t0: SetStateContravariant<"a"> = {} as SetStateContravariant<"a" | "b"> // compiles
let t1: SetStateContravariant<"a" | "b"> = {} as SetStateContravariant<"a"> // does not compile
let t2: SetStateBivariant<"a"> = {} as SetStateBivariant<"a" | "b"> // compiles
let t3: SetStateBivariant<"a" | "b"> = {} as SetStateBivariant<"a"> // also compiles So now we can make interface Store<T> {
getState: () => T
setState(t: T): void
}
declare const useStore:
<S extends Store<unknown>>(t: S) =>
S extends { getState: () => infer T } ? T : never
declare const myStore: Store<{ count: number }>
useStore(myStore) // now it compiles Now you might be wondering how was this not a problem in Zustand? It's because Zustand's type SetState<T> = (t: T) => void
type SetStateWithPartial<T> = (t: Partial<T> | T) => void
// SetStateWithPartial has different behavior than SetState
// but only when objects are invovled
let t0: SetState<object> = {} as SetState<{ count: number }> // does not compile
let t1: SetState<object> = {} as SetStateWithPartial<{ count: number }> // compiles
// different behavior
let t2: SetState<unknown> = {} as SetState<{ count: number }> // does not compile
let t3: SetStateWithPartial<unknown> = {} as SetStateWithPartial<{ count: number }> // also does not compile
// same behavior
let t4: SetState<unknown> = {} as SetState<"a"> // does not compile
let t4: SetStateWithPartial<unknown> = {} as SetStateWithPartial<"a"> // also does not compile
// same behavior So Zustand kinda fluked this and got lucky because not only it uses type Store<T> =
{ setState:
<R extends boolean | undefined>
( wholeWhenReplacingElsePartial: R extends true ? T : Partial<T>
, isReplacing?: R
) => void
}
type DevtoolsStore<T> =
{ setState:
<R extends boolean | undefined>
( wholeWhenReplacingElsePartial: R extends true ? T : Partial<T>
, isReplacing?: R
, name?: string
) => void
}
let t0: Store<{ count: number }> = {} as DevtoolsStore<{ count: number }> // compiles
let t1: Store<object> = {} as DevtoolsStore<{ count: number }> // doesn't compile but should So now with bivariant type Store<T> =
{ setState
<R extends boolean | undefined>
( wholeWhenReplacingElsePartial: R extends true ? T : Partial<T>
, isReplacing?: R
): void
}
type DevtoolsStore<T> =
{ setState
<R extends boolean | undefined>
( wholeWhenReplacingElsePartial: R extends true ? T : Partial<T>
, isReplacing?: R
, name?: string
): void
}
let t0: Store<{ count: number }> = {} as DevtoolsStore<{ count: number }> // compiles
let t1: Store<object> = {} as DevtoolsStore<{ count: number }> // also compiles Hence the subject of the commit says "make setState bivariant to make the state covariant" (funny how effectively we can communicate using a formal language, it takes a few words to pack all of this explanation xD) Also I simplified the < Nt extends (R extends true ? T : Partial<T>)
, R extends boolean | undefined
>
( partial: Nt
, replace?: R
) => void ...we can simply inline <R extends boolean | undefined>
( partial: R extends true ? T : Partial<T>
, replace?: R
) => void |
You are too optimistic about this. The maintainers skills are not high enough as you expect to modify those code (and understand your detailed documents). Moreover, other contributors/users can't read the code. How would they become confident with the code they don't understand? It's like a different programming language for someone.
I know, and this means if someone creates a new middleware at their own, the type inference will break, right? Creating own middleware and using I don't have any solution. Just feels like it's too complicated, and if you have any idea to compromise, it's worth considering. |
I don't fully understand invariant/covariant things, but the commit is readable. If there are tests to cover, I could follow it. |
…iant too (I forgot to make this bivaraint)
No, here's the scenario..
So it won't break anything, the type inference will still work, everything will be the same, just the types will become less strict. Let me clarify the typescript limitation a bit... type State = { count: number }
declare const setState:
<R extends boolean | undefined>
( partial: R extends true ? State : Partial<State>
, replace?: R
) =>
void
setState({}, true)
// doesn't compile, great, it's strict and prevents runtime exceptions
declare const withDevtools:
<F extends (...a: never[]) => unknown>
(f: F) =>
// code same as the fallback branch in `StoreSetStateWithAction`
F extends (...a: infer A) => infer R
? (...a: [...a0: TakeTwo<A>, name?: string]) => R
: never
type TakeTwo<T> =
T extends [] ? [undefined, undefined] :
T extends [unknown] ? [...a0: T, a1: undefined] :
T extends [unknown?] ? [...a0: T, a1: undefined] :
T extends [unknown, unknown] ? T :
T extends [unknown, unknown?] ? T :
T extends [unknown?, unknown?] ? T :
T extends [infer A0, infer A1, ...unknown[]] ? [A0, A1] :
T extends [infer A0, (infer A1)?, ...unknown[]] ? [A0, A1?] :
T extends [(infer A0)?, (infer A1)?, ...unknown[]] ? [A0?, A1?] :
never
const setStateWithDevtools = withDevtools(setState)
// all we wanted to do is add an argument at 2nd index
// we did that but now we also lost all generics
// (hover over `setStateWithDevtools`)
setStateWithDevtools({}, true)
// compiles, not that great as it's not strict and doesn't prevent runtime exceptions
I just want to make sure you realize there's no pressure from my side, only suggestions. To quote from #662's description...
My job is to offer you the most correct code possible. Your job is keep whatever code you like, which means I tailor the code according to your needs. I can at best suggest you, the decision will be yours. So here given you find I just committed " You have two options here:
The first option takes away the strict replace feature for making the code less complicated. The second option makes the code more complicated for the strict replace feature. My suggestion is still this...
...and it's only a suggestion, really. You should do what is right according to you, because everyone has different OSS philosophies, different judgements of the risk-to-reward ratio, different responsibilities, etc. Any my apologies if my suggestions came across as some kind of pressure to accept them. Also if the code is still complicated for you (given you tell me which part) then I can still make more compromises and make it less complicated, feel free to tell me. |
I see. Yeah, this applies to
I missed this part. We can suggest using devtools at leaf level.
It was the start of this discussion, but in general, it's too complicated to be too good.
I'll see the code again. But, probably more I read, more I get how it works, and "wow, it's good".
From the lib maintaining perspective, adding feature (including strict checks) is easier than removing it. So, I will keep fb624ac commit in this PR. Alternatively, as having an option is good, I will open a new PR for adding "strict replace" and I will consider merging after releasing v4 and getting more feedback. Or, maybe you want to open the PR, because it's clearer about your work. Would you? |
No not if the middleware author writes correct types (just like we write correct types for
Yes but the other way round (as I wrote "
Ha thanks!
No you got it backwards. Adding a strict check is a breaking change, so it's easier to remove it than to add it. So if you publish v4 without strict replace then you can only add it in v5 as it's a breaking change. But if you publish v4 with strict replace then you can remove it in v4.x because it won't be a breaking change. Again, idm anything just correcting you. And sure I can open a PR if you wish. |
oops, "at the top level", i see.
That's good to know.
Yes, please. I will (re-)consider about it after releasing v4-rc.0 with this PR before releasing final v4. And please make this PR ready for merge. We'd like to move forward. |
LGTM |
diff -urw dist-76eeacb/esm dist-9d6e7e2/esmdiff -urw dist-76eeacb/esm/index.js dist-9d6e7e2/esm/index.js
--- dist-76eeacb/esm/index.js 2022-04-18 08:56:01.000000000 +0900
+++ dist-9d6e7e2/esm/index.js 2022-04-18 08:57:05.000000000 +0900
@@ -8,11 +8,13 @@
useDebugValue(slice);
return slice;
}
-function create(createState) {
+const createImpl = (createState) => {
const api = typeof createState === "function" ? createStore__default(createState) : createState;
const useBoundStore = (selector, equalityFn) => useStore(api, selector, equalityFn);
Object.assign(useBoundStore, api);
return useBoundStore;
-}
+};
+const create = (createState) => createState ? createImpl(createState) : createImpl;
+var create$1 = create;
-export { create as default, useStore };
+export { create$1 as default, useStore };
diff -urw dist-76eeacb/esm/middleware.js dist-9d6e7e2/esm/middleware.js
--- dist-76eeacb/esm/middleware.js 2022-04-18 08:55:57.000000000 +0900
+++ dist-9d6e7e2/esm/middleware.js 2022-04-18 08:57:01.000000000 +0900
@@ -1,3 +1,5 @@
+import { produce } from 'immer';
+
var __defProp$1 = Object.defineProperty;
var __getOwnPropSymbols$1 = Object.getOwnPropertySymbols;
var __hasOwnProp$1 = Object.prototype.hasOwnProperty;
@@ -14,7 +16,7 @@
}
return a;
};
-const redux = (reducer, initial) => (set, get, api) => {
+const reduxImpl = (reducer, initial) => (set, get, api) => {
api.dispatch = (action) => {
set((state) => reducer(state, action), false, action);
return action;
@@ -22,9 +24,9 @@
api.dispatchFromDevtools = true;
return __spreadValues$1({ dispatch: (...a) => api.dispatch(...a) }, initial);
};
+const redux = reduxImpl;
-function devtools(fn, options) {
- return (set, get, api) => {
+const devtoolsImpl = (fn, options) => (set, get, api) => {
const devtoolsOptions = options === void 0 ? {} : typeof options === "string" ? { name: options } : options;
let extensionConnector;
try {
@@ -40,10 +42,11 @@
const extension = extensionConnector.connect(devtoolsOptions);
let isRecording = true;
api.setState = (state, replace, nameOrAction) => {
- set(state, replace);
+ const r = set(state, replace);
if (!isRecording)
- return;
+ return r;
extension.send(nameOrAction === void 0 ? { type: devtoolsOptions.anonymousActionType || "anonymous" } : typeof nameOrAction === "string" ? { type: nameOrAction } : nameOrAction, get());
+ return r;
};
const setStateFromDevtools = (...a) => {
const originalIsRecording = isRecording;
@@ -117,7 +120,7 @@
});
return initialState;
};
-}
+const devtools = devtoolsImpl;
const parseJsonThen = (stringified, f) => {
let parsed;
try {
@@ -129,7 +132,7 @@
f(parsed);
};
-const subscribeWithSelector = (fn) => (set, get, api) => {
+const subscribeWithSelectorImpl = (fn) => (set, get, api) => {
const origSubscribe = api.subscribe;
api.subscribe = (selector, optListener, options) => {
let listener = selector;
@@ -152,8 +155,9 @@
const initialState = fn(set, get, api);
return initialState;
};
+const subscribeWithSelector = subscribeWithSelectorImpl;
-const combine = (initialState, create) => (set, get, api) => Object.assign({}, initialState, create(set, get, api));
+const combine = (initialState, create) => (...a) => Object.assign({}, initialState, create(...a));
var __defProp = Object.defineProperty;
var __getOwnPropSymbols = Object.getOwnPropertySymbols;
@@ -196,7 +200,7 @@
};
}
};
-const persist = (config, baseOptions) => (set, get, api) => {
+const persistImpl = (config, baseOptions) => (set, get, api) => {
let options = __spreadValues({
getStorage: () => localStorage,
serialize: JSON.stringify,
@@ -304,5 +308,15 @@
hydrate();
return stateFromStorage || configResult;
};
+const persist = persistImpl;
+
+const immerImpl = (initializer) => (set, get, store) => {
+ store.setState = (updater, replace, ...a) => {
+ const nextState = typeof updater === "function" ? produce(updater) : updater;
+ return set(nextState, replace, ...a);
+ };
+ return initializer(store.setState, get, store);
+};
+const immer = immerImpl;
-export { combine, devtools, persist, redux, subscribeWithSelector };
+export { combine, devtools, immer, persist, redux, subscribeWithSelector };
diff -urw dist-76eeacb/esm/vanilla.js dist-9d6e7e2/esm/vanilla.js
--- dist-76eeacb/esm/vanilla.js 2022-04-18 08:55:56.000000000 +0900
+++ dist-9d6e7e2/esm/vanilla.js 2022-04-18 08:57:00.000000000 +0900
@@ -1,4 +1,4 @@
-function createStore(createState) {
+const createStoreImpl = (createState) => {
let state;
const listeners = /* @__PURE__ */ new Set();
const setState = (partial, replace) => {
@@ -18,6 +18,7 @@
const api = { setState, getState, subscribe, destroy };
state = createState(setState, getState, api);
return api;
-}
+};
+const createStore = (createState) => createState ? createStoreImpl(createState) : createStoreImpl;
export { createStore as default }; JS change seems minimal, but realized that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this in. Thanks so much for your work!
You're welcome, and thanks for the opportunity and your patience! |
Okay @dai-shi now you have two options:
A. This PR (not complete yet)
Makes the change as minimal as possible, just adds the types proposed in #710, zero change in runtime code.
B. #662
Not only adds #710 but also rewrites the whole codebase to drastically increase the code quality. Don't mind me but the current code is quite a mess, I like to write code for fun so I thought let's rewrite this whole thing nicely. Now this obviously will produce relatively large and unhelpful diffs, nothing can be done about that. It would be much easier to review if you compare the files locally without the red-green diff.
But now I have a small condition: I'll remove prettier and some lint rules, but don't worry you can add them back but you'll have to do it yourself (you have the edit rights). Consider this a little treat that I'm giving myself :P I hope you won't mind.
Let me know which of the two options you prefer. If you think increasing the code quality is worth taking the pain to review, I'd suggest you go with the second option. I don't mind whichever you pick.