-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
add vike-react-zustand #57
base: main
Are you sure you want to change the base?
Conversation
Yes, that's definitely valuable 💯
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?
👍
Yes, I think a key is always required in the context of SSR. Just like it's required for
I don't know yet 👀 maybe we can mull over this once we've got the basic functionalities working (i.e. the state hydration thingy). |
added |
Should we pass |
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 |
👍 Good idea. How about we name it I was also thinking that, for |
47123f4
to
2b7ab52
Compare
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, 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 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 |
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 It's not that misleading anymore. (maybe 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. |
@@ -33,7 +33,7 @@ export default function VikeReactZustandWrapper({ pageContext, children }: VikeR | |||
for (const [key, store] of stores) { | |||
// Trick to make import.meta.env.SSR work direclty on Node.js (without Vite) | |||
// @ts-expect-error | |||
import.meta.env ??= { SSR: true } | |||
import.meta.env = { SSR: true } |
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.
Doesn't work, import.meta.env is not statically evaluated in client code in dev, it would set SSR:true in browser.
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.
I didn't test it so I may be wrong, but I'm thinking it should work because import.meta.env.SSR
is statically removed (enabling code splitting) whenever Vite transpiles code. I.e. the runtime value import.met.env.SSR
is only ever used for externalized server-side.
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.
import.meta.env is not statically evaluated in client code in dev
Ah, sorry, didn't read that. Ok, let's revert then.
859fc5b
to
412017a
Compare
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.