-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Proposal: Encoding store mutations type-level #710
Comments
wow, it's too long. I don't follow the later part. I like the approach with the phantom mutation type. That's something I wanted to achieve with #601 originally. so, my counter proposal is to live with the "contextual aware function" limitation of TS behavior, and give up propagation mutation type top to bottom. In such cases, developers need to explicitly type store at the top. the question is if the TS error message is fairly readable. otherwise, we'd need a good documentation around this. what do you think? do you have any other proposal to balance between the complexity and DX? |
Haha that's okay, I can help you if you tell me which part exactly. I agree on the complexity part but it's not userland, with // https://tsplay.dev/WJRdgW
let storeOut = createWithState<{ count: number }>()(
withA("a",
withB("b",
storeIn => ({ count: 0 })
)
)
)
// `storeOut` and `storeIn` are `Write<Write<Store<{ count: number }>, { a: string }, { b: string }>> And there are many problems with the existing phantom mutation type approach that leads to a worse DX compared to this proposal, some examples... (checkout to This proposal is very robust, hence these two compile... And there's nothing to lose with this proposal, even the caveat that I mentioned in the proposal is gone, it works with extracting middlewares too. // https://tsplay.dev/mpvY7w
let withBBounded = withB("b", storeIn1 => ({ count: 0 }))
let storeOut1 = create(withA("a", withBBounded))
let storeOut2 = create(
withA("a",
withB("b",
storeIn2 => ({ count: 0 })
)
)
)
// `storeIn1` is `Write<Store<object>, { b: string }>
// `storeOut1`, `storeIn2`, `storeOut2` are `Write<Write<Store<object>, { a: string }>, { b: string }>> Here all stores have correct types, the one you'd expect them to have. So in my eyes there are no downsides to this proposal. The only downside is the added complexity and migration BUT only for the people who author middlewares themselves. I'm not a zustand user but I think there will be few. We can send PRs to popular third-party middlewares (if any). Btw This is how authoring a middleware looks, a bit complex but no rocket science as such. This proposal with If you're not confident about it already how about I go ahead with this, then you see the final piece—the tests, the changes for migration, etc—and if you don't like I'll revert it. Wdyt? |
maybe I misunderstood your nuance. so, if you are comfortable with continuing your approach, you can go on. My concerns are a) if I can understand the final
don't you call your approach phantom mutation type? or is it phantom mutation type + alpha? |
Oh okay cool!
You might not understand it fully but I think you'll be able to understand 90% of it, I'll help you out. The tests might not pass as they are of course, but with migration they'll. I'll try to minimize breaking changes and make the migration as easy as possible.
I don't call it by a certain name haha. But let's call it "higher kinded mutators". Ahhh looks like when you said "I like phantom mutation type approach" you meant didn't mean the old approach (like in #660, hence I was comparing them) but the overall approach including this proposal, gotcha. That was the misunderstanding I guess haha. |
yeah. |
Middlewares in zustand can and do mutate the store, which makes it impossible to type them with without some workaround, this proposal tries to encode mutations type-level to make zustand almost 100% type-safe and still extensible.
I'll walk you through how this came to be, because I think it makes one understand how it works and why certain elements of it are necessary.
PR #662 already implements this proposal so you can try it out if you want. And here's the final playground to try things out.
Index
This is less of a "table of content" and more of an "index", meaning the whole thing is single-read and there are no independent "sections" per se, they are made as such to aid the reader to track their progress because this document is rather too long to be a single-read.
Independent mutations
Consider this scenario (I'll be making the api simpler only for the explanation, there are no api changes required)...
Here both
storeOut
andstoreIn
should be of typeStore<unknown> & { a: string } & { b: string }
(we don't care about inferring the state{ count: number }
for now)So how would we approach this? First let's take a simpler version
One could begin with something like...
And it works.
storeOut
andstoreIn
areStore<unknown> & { a: string }
. But why does it work? Intuitively you could think that had the store been immutable the middlewares would have returned the mutation isn't it? That's what we're doing here, the middlewares not only return the initializer but also the mutation to the store.Forwarding mutations up the tree
Okay now let's see if it works for a more complex example...
Hmm, it doesn't work.
storeOut
isStore<unknown> & { b: string }
andstoreIn
isStore<unknown> & { a: string }
. Why is that? Because were aren't accommodating the fact that the "child" ofwithB
iewithA
itself comes with its mutation so we need to carry forward mutations up the tree. Let's do that1...So when the child itself has a mutation
M
(ie when the initializer has{ mutation?: M }
) we carry it forward as{ __mutation?: M & { a: A } }
instead of just{ __mutation?: { a: A } }
.Also the
extends {}
constraint onM
is so that it doesn't get fixated toundefined
because of the?
in{ __mutation?: M }
.Forwarding mutations down the tree
Okay so now the mutation is forwarded upstream and
storeOut
isStore<unknown> & { a: string } & { b: string }
. ButstoreIn
is stillStore<unknown> & { a: string }
. That's because we have only forwarded mutations upstream, we also need to forward the mutations downstream. Let's see how we can do that...But it doesn't compile. Why so? Let me first show you a minimal equivalent of the above...
Now what's happening here? Why does it not compile? The problem is TypeScript can't contextually infer
S
, it eagerly resolves it toStore<unknown>
. And why does it do that? Well it'll take me rather too long to develop what I know intuitively so this comment is the best I can offer right now.Long story short it doesn't work, we need a workaround. Which looks something like this...
So instead of passing the mutation from parent as
(store: Store<T> & Pm) =>
we're doing(store: Store<T>, __mutation: Pm) =>
which in spirit is the same thing. And instead ofPm & { a: A }
we trick the compiler by doingUnionToIntersection<Pm | { a: A }>
which is effectively resolves to the same thing.Why does all this works? Well, I kinda vaguely know it in my "feels" but it's going to take me a lot of experimentation to come up with a good enough theory, so let's just skip that xD
So now finally
storeOut
andstoreIn
both areStore<unknown> & { a: string } & { b: string }
Ordered writes
What if we have a scenario like this...
Here with our existing approach
storeOut
andstoreIn
would beStore<unknown> & { a: number } & { a: string }
when it should have beenStore<unknown> & { a: string }
. That means we need to "write" instead of just intersecting and order of mutations matters too. Let's see how we can do that...Mps
happen before because they are come from the parent,Mcs
happen after because they come from the child.Note that we are doing
= []
forMcs
andMps
because previously when there's no{ __mutation }
found (like in case of() => ({ count: 0 })
) the compiler would resolve them to their constraints ie{}
which also happened to be the default. Now the default is[]
so we need to make it explicit. So technically speaking previously too we should have hadMc extends {} = {}
but that's almost same as writingMc extends {}
.So now
storeOut
andstoreIn
areMutate<Store<unknown>, [{ a: string }, { a: number }]>
except that in case ofstoreOut
,number
gets narrowed to1
. We can fix that by blocking inference ofA
at the places it isn't supposed to be inferred from...So now we're inferring
A
only from the argument, as it should be. And nownumber
no longer gets narrowed to1
.Also remember that child mutations can also happen before the current one, example...
In this case we'd expect
storeOut.a
to benumber
instead ofstring
unlike the previous case. Depending on the implementation, the author can type the middleware accordingly, in this case we're first doing the child mutations so the type becomes...Subtractive mutations
Although rare but it's important to note that now mutations can make a store no longer extend
Store<T>
, in that case some cases that should compile won't compile, example...If you scroll at the end of the error message you'd see it says "'undefined' is not assignable to type '(t: unknown) => void'" that's because
withA
's type says it requiresStore<T>
when in fact it doesn't useset
and only sets a propertya
. Previously, we moved from(store: Store<T> & Pm) =>
to(store: Store<T>, __mutation: Pm) => void
because the former didn't work. But now we can go back to that and useMutate<Store<T>, Mps>
which would solve our problem and make it compile...But what if
withA
actually requiresset
and want the store to be of typeStore<T>
? In that case we can specify our requirements and then we get the original error back which is now warrant...It's important to only specify only what we require in the implementation, for example if
withA
only cared aboutget
it should specify only that would then make our code compile again as it should...Dependent mutations
At any point if our mutation has a
T
on a covariant position, it no longer compiles. Basically if the middleware (mutation) "gives" or "returns" the state it meansT
is on a covariant position (like in case ofpersist
,onHydrate
andonFinishHydration
"give" the state), it won't work. Here's a simple example...If you scroll to the end of the huge error you'd find something like
WithSerialized<unknown> is not assignable to WithSerialized<never>
. The thing for some reason typescript (this could be a bug) resolvesT
tonever
instead ofunknown
assuming it to be contravariant.I can make it compile by making all signatures bivariant but still
storeIn
would haveWithSerialized<never>
instead ofWithSerialized<unknown>
so that doesn't work. It'd work ifT
is already concrete either via user explicitly typing it or via a helper. These solutions could work but let's actually solve the problem.Higher kinded mutations
One thing is clear that we can't have
T
directly on the mutations, that is to say we can't have{ a: A, t: T }
as a mutation so instead we'll use an emulated higher kinded type(T, A) => { a: A, t: T }
and injectT
(and an optional payload likeA
in this case)...So instead of passing around mutations
M
we pass "functions"(T, A) => M
that takeT
and some optional payload. So writing......is in spirit same as writing a higher kinded type...
So
StoreMutations
is a collection of "mutation creators". And now as you can see in the diffM
becomesStoreMutations<T, A>[I & StoreMutationIdentifier]
2. WritingStoreMutations<T, A>[I]
is in spirit writingstoreMutations[i](t, a)
which would returnm
.StoreMutations
is also a "global" collection so it can be augmented anywhere and new mutations can be added. It works across files thanks to module augmentation and declaration merging, so in reality adding a mutation would look like this...Higher kinded mutators
Now that we are using higher kinded types, we can simplify things a bit and instead of
newStore = overwrite(oldStore, storeMutations[mi](tFromOldStore, a))
just simply donewStore = storeMutators[mi](oldStore, a)
.Zustand-adapted version
Here's a version that is compatible with zustand's api (same as in #662), uses unique symbols for identifiers, a reusable
StoreInitializer
, etc and an example middleware.To-dos
This is still sort of incomplete here are some things left to figure out...
Caveats
It doesn't compile if you don't write all middlewares in place because it relies on contextual inference, meaning this doesn't compile...
... but this does ...
I recently noticed this caveat so I haven't really worked on it. Maybe I could make it at least compile will a little less accuracy in types.
Other
createWithState
? Probably a good idea.createWithState
do we still need higher kinded types? Maybe, maybe not.Action point
I'd like to know if this seems like a good idea or not, if yes then I'll continue with the to-dos. If no then how far should we go wrt to types?
Thanks for reading all this! Hopefully I've not made much mistakes. Feel free to ask questions
Footnotes
Note that the diffs through out the document are just gists, they are not exact it excludes some obvious changes elsewhere (like in
create
) and also renames variables, changes formatting etc. The diff is minimal enough to get the crux of what changed. Go to the playground link to see the full change ↩The
& StoreMutationIdentifier
is to just satisfy the compiler thatI
indeed is a key ofStoreMutations
(asX & Y
will always be a subtype ofY
).I & StoreMutationIdentifier
is same asI
becauseI
already is a subtype ofStoreMutationIdentifier
, eg"WithSerialized" & ("WithSerialized" | "WithA")
is"WithSerialized"
. ↩The text was updated successfully, but these errors were encountered: