Open nitedani opened 8 months ago
What do you think about this?
Yes, that's definitely valuable :100:
tricky
Yes, in particular sending the initial store state set by SSR to the client-side. Speaking of which, this part doesn't work yet right?
The current implementation doesn't support multiple stores, but I think it's already useful.
:+1:
For multiple stores, support can be added using a unique key passed to
createUseStore
, then the key can be used as an identifier to get the right store from the context.
Yes, I think a key is always required in the context of SSR. Just like it's required for vike-react-query
.
Or a completely different implementation, using + files.
I don't know yet :eyes: maybe we can mull over this once we've got the basic functionalities working (i.e. the state hydration thingy).
@brillout
the state hydration thingy
added
Should we pass everything
to the client on initial navigation, or only the return value of server
?
https://github.com/vikejs/vike-react/pull/57/files#diff-27a888a36f2dca710aeb519e21c30e5dad4ae192110374c14481703b23a7f2afR22-R24
I don't think we have a choice? AFAICT the entire state used for hydration should be exactly the same than the state used for SSR.
Ok. I think it's ready. After using typeof window === "undefined"
multiple times, I had an idea: maybe the environment can be set on the pageContext by Vike: pageContext.env: 'client' | 'server'
and then we could use that instead.
Ok. I think it's ready. After using
typeof window === "undefined"
multiple times, I had an idea: maybe the environment can be set on the pageContext by Vike:pageContext.env: 'client' | 'server'
and then we could use that instead.
👍 Good idea. How about we name it pageContext._env
so that end users are incentivized to use import.meta.env.SSR
instead so that they get code splitting.
I was also thinking that, for vike-react-query
, Vike could set import.meta.env.DEV
.
I think the following proxy is misleading:
new Proxy(useStore, {
get(target, p: keyof ReturnType<typeof createZustand>) {
return target()[p]
},
apply(target, _this, [selector]) {
return target()(selector)
}
})
Because it only works when respecting React's hook rules. And it doesn't really add any value, so how about we remove that proxy?
Instead, I think what we could do is this:
// Only works on the client-side
import { getStore } from 'vike-react-zustand/client'
// `store` is the store object provided by Zustand
const store = getStore()
I agree, it's misleading because it's bound to the context. The issue is not solved with your idea though, getStore
can still be called outside of the context. The function name should start with use
to signal to the user that it should be used in a component. I think the current proxy has that advantage.
RIght now you can do:
function Component(){
useStore.getState()
}
Can't do:
function Component(){
}
useStore.getState()
Can't do:
function Component(){
if(something)
useStore.getState()
}
But can't do neither:
function Component(){
}
getStore()
Maybe, we could export a function useStoreApi
or something?
Or, we could remove the functionality, and don't let users use the store api at all?
Or change the throw new Error("Store has no provider")
to a usage warning message?
Or, I don't know about this, but maybe there is a way to use the context without respecting the rule of hooks?
Either way, I agree we should remove the proxy for now, and maybe find a solution later to expose the store api.
What do you think about the change? I added useStoreApi
and removed the Proxy. The type of create
is still misleading on userland though, because we 1:1 use the zustand type. I also don't know yet how we can cleanly make useStoreApi
compatible with multiple stores in the future. Maybe require users to call it like: useStoreApi(useStore)
and we can set useStore.__storeKey
in the library.
Started adding support for multiple stores, but I'll leave it like this for now. Just wanted to know if it will be possible to do. There is only the hardcoded "default" store right now.
Added StoreHookOnly
and StoreApiOnly
types.
StoreHookOnly
is only the function,
StoreApiOnly
is only the getState|setState|subscribe|destroy
.
It's not that misleading anymore. (maybe useStoreApi(useStore)
)
import { create, useStoreApi } from 'vike-react-zustand'
interface Store {
counter: number
}
const useStore = create<Store>((set, get) => ({counter: 1})) // StoreHookOnly<Store>
useStore.getState // Property 'getState' does not exist on type 'StoreHookOnly<Store>'
useStore() // Store
useStore(s => s.counter) // number
const api = useStoreApi(useStore) // StoreApiOnly<Store>
api.getState() // Store
api() // This expression is not callable.
@nitedani Hi Niteda, does Vike have any problem merging your PR? I think we are still waiting for you.
It's the other way around: this PR is blocked by a Vike feature we didn't implement yet.
we are still waiting for you
Sponsoring welcome in case your company would be interested in getting higher priority feature requests with an ETA.
What do you think about this? Abstracting the creation of the context for the store, is tricky. The current implementation doesn't support multiple stores, but I think it's already useful. For multiple stores, support can be added using a unique key passed to
createUseStore
, then the key can be used as an identifier to get the right store from the context.Or a completely different implementation, using + files.